New issue
Advanced search Search tips

Issue 789669 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug


Show other hotlists

Hotlists containing this issue:
XXX


Sign in to add a comment

Layout Test MediaStreamTrack-gc-no-crash.html flaky when using video capture service

Project Member Reported by chfremer@chromium.org, Nov 29 2017

Issue description

See reverted CL which activates the video capture service by default: https://chromium-review.googlesource.com/c/chromium/src/+/719416

After the CL landed, the test failed 3 times in a row starting at build 
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/12301

The 4th time it passed, which was in build 
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/12304

With the next build the CL was reverted, and it looks like the test always passed since.

There might be a (possibly existing) race condition that is typically never hit, but gets hit more often with some timing changes when the new service is activated.
 
Update: The test failure is not because of the test itself failing, but is triggered by the leak detector that runs after the test has finished. 

I was able to reproduce it locally. Turns out that a command-line flag --enable-leak-detection is needed to run the leak detector after the test, which is what happens on the bot.

With that, I was able to conclude that the leaked objects are due to the video capture service having a 5 second timeout before shutting down. When making sure that the service has shut down before the test finishes, the leaks are gone.

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1702c9095ed676cd89f6bac4c1e522dd9a32e3dc

commit 1702c9095ed676cd89f6bac4c1e522dd9a32e3dc
Author: Christian Fremerey <chfremer@chromium.org>
Date: Tue Dec 05 16:59:04 2017

Make test MediaStreamTrack-gc-no-crash deterministic

The Layout Test MediaStreamTrack-gc-no-crash.html was non-determinstic in that
it depend on a race between the JavaScript side declaring the asynchronous test
"finished" and the callback to GetUserMedia arriving.

It appears there were at least 3 possible outcomes. The first case was that
the finishJSTest() would be invoked before the call to GetUserMedia
would do anything much, and the test would essentially verify nothing.
The second case was that when finishJSTest() was invoked, GetUserMedia would
have reach a point where it would have temporarily taken shared ownership
of the blink document but the callbacks would not yet have been invoked.
The third case would be that finishJSTest() would be invoked after the
callback to GetUserMedia had finished running.

It looks like in practice, at least on my local machine, the first case
was the only one reached so far.

When trying to activate the new video capture service with [1], the
timing changed so that the second case would typically be reached. This case
would lead to the leak detector running at the end of the test reporting
leaked blink objects and failing the test. I verified that the same would
happen without activating the video capture service, simply by making the
call to FakeVideoCaptureDeviceFactory::GetDescriptors() taking a bit longer,
via PlatformThread::Sleep().

After this CL, the test should deterministically always reach the third case.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/719416

Bug:  789669 
Test: third_party/WebKit/Tools/Scripts/run-webkit-tests --target gn_release --enable-leak-detection fast/mediastream/MediaStreamTrack-gc-no-crash.html
Change-Id: I17c39e66cfd8c4af89f8f9b078d6512feaedbe6d
Reviewed-on: https://chromium-review.googlesource.com/806442
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521723}
[modify] https://crrev.com/1702c9095ed676cd89f6bac4c1e522dd9a32e3dc/third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-gc-no-crash-expected.txt
[modify] https://crrev.com/1702c9095ed676cd89f6bac4c1e522dd9a32e3dc/third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-gc-no-crash.html

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0c5b69e33aaf67089ff8dc584bac670b0168c62

commit f0c5b69e33aaf67089ff8dc584bac670b0168c62
Author: Christian Fremerey <chfremer@chromium.org>
Date: Wed Dec 06 21:48:43 2017

Make blink layout-test argumen-types.html deterministic

This is a second instance of the same issue and fix as reviewed in
https://chromium-review.googlesource.com/c/chromium/src/+/806442.

TBR=emircan@chromium.org,hajimehoshi@chromium.org

Bug:  789669 
Test: third_party/WebKit/Tools/Scripts/run-webkit-tests --target gn_release --enable-leak-detection fast/mediastream/argument-types.html
Change-Id: If16c19395209a19a4548491f8b3e04893915adc2
Reviewed-on: https://chromium-review.googlesource.com/811865
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522206}
[modify] https://crrev.com/f0c5b69e33aaf67089ff8dc584bac670b0168c62/third_party/WebKit/LayoutTests/fast/mediastream/argument-types-expected.txt
[modify] https://crrev.com/f0c5b69e33aaf67089ff8dc584bac670b0168c62/third_party/WebKit/LayoutTests/fast/mediastream/argument-types.html

Status: Fixed (was: Started)

Sign in to add a comment