Bug 102247 - webgl-background-color.html started failing
Summary: webgl-background-color.html started failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-14 10:02 PST by Dimitri Glazkov (Google)
Modified: 2012-11-15 16:40 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.35 KB, patch)
2012-11-14 12:28 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (1.43 KB, patch)
2012-11-14 13:45 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 James Robinson 2012-11-14 12:28:10 PST
Created attachment 174223 [details]
Patch
Comment 2 James Robinson 2012-11-14 12:35:53 PST
The issue here was actually a timeout due to chromium http://src.chromium.org/viewvc/chrome?view=rev&revision=167537.  WebViewHost's d'tor navigates the page (synchronously) to about:blank:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Tools/DumpRenderTree/chromium/WebViewHost.cpp&exact_package=chromium&q=~WebViewHost&type=cs&l=1446

one consequence of which is disabling compositing mode, which triggers this finishAllRendering() call:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&exact_package=chromium&q=WebViewImpl.cpp&type=cs&l=3946

which attempts to make a synchronous call to the compositor thread.  Since the compositor thread is destroyed first, this call went nowhere and the main thread simply sat blocked until it was killed by the harness.

I think the reason http://src.chromium.org/viewvc/chrome?view=rev&revision=167537 tickled this was because of some subtleties in chromium's thread shutdown where the thread was living for longer, but I'm still tracking down exactly how this happened.
Comment 3 James Robinson 2012-11-14 12:37:36 PST
This patch also makes TestShell's behavior more similar to chrome's, where the compositor thread always outlives WebView instances.
Comment 4 Adrienne Walker 2012-11-14 12:45:56 PST
Comment on attachment 174223 [details]
Patch

R=me.  This really needs a better comment.
Comment 5 Dimitri Glazkov (Google) 2012-11-14 13:32:46 PST
(In reply to comment #4)
> (From update of attachment 174223 [details])
> R=me.  This really needs a better comment.

I think this change needs a meme.
Comment 6 James Robinson 2012-11-14 13:45:35 PST
Created attachment 174246 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-11-14 14:09:45 PST
Comment on attachment 174246 [details]
Patch for landing

Clearing flags on attachment: 174246

Committed r134667: <http://trac.webkit.org/changeset/134667>
Comment 8 WebKit Review Bot 2012-11-14 14:09:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Ojan Vafai 2012-11-14 16:15:28 PST
Is there a way to add asserts instead of just a comment?
Comment 10 James Robinson 2012-11-14 17:28:01 PST
They aren't passing yet, although I think r134667 was a solid bug fix and should cut down on flaky crashes.  There's one more race that https://codereview.chromium.org/11363254 should fix.  Leaving open until it goes green...
Comment 11 James Robinson 2012-11-15 16:40:34 PST
Fixed by http://src.chromium.org/viewvc/chrome?view=rev&revision=168037, removed failing expectation in http://trac.webkit.org/changeset/134857