Bug 56148 - [chromium] Graphics Context is not properly recovered if the GPU process dies.
Summary: [chromium] Graphics Context is not properly recovered if the GPU process dies.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Alexey Marinichev
URL:
Keywords:
Depends on:
Blocks: 56620
  Show dependency treegraph
 
Reported: 2011-03-10 15:42 PST by Alexey Marinichev
Modified: 2011-03-17 18:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.75 KB, patch)
2011-03-10 15:50 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2011-03-10 18:15 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (5.29 KB, patch)
2011-03-11 16:22 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Rebased to top of tree. (5.43 KB, patch)
2011-03-11 17:05 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Marinichev 2011-03-10 15:42:21 PST
Because of the race in the lost context detection, recreated context will be thrown away when GPU process restarts.
Comment 1 Alexey Marinichev 2011-03-10 15:50:22 PST
Created attachment 85397 [details]
Patch
Comment 2 Kenneth Russell 2011-03-10 16:54:03 PST
Comment on attachment 85397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85397&action=review

Good work fixing these deep issues; r- for a couple of relatively minor concerns.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:90
> +    PassRefPtr<LayerChromium> rootLayer() { return m_rootLayer.release(); }

I've already given you this feedback offline, but in this form this method should be named something like "releaseRootLayer".

> Source/WebKit/chromium/src/WebViewImpl.cpp:2436
> +            getCompositorContextAttributes(), m_page->chrome(), GraphicsContext3D::RenderDirectlyToHostWindow);

Are any similar improvements needed in WebGLRenderingContext or WebGLLayer?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2443
> +        // TODO(amarinichev): In MacOS newContext->reshape method needs to be

WebKit uses FIXME without an owner, not TODO.
Comment 3 Alexey Marinichev 2011-03-10 18:15:58 PST
Created attachment 85418 [details]
Patch
Comment 4 Kenneth Russell 2011-03-11 14:04:03 PST
Comment on attachment 85418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85418&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1087
> +    if (m_recreatingGraphicsContext) {
> +        reallocateRenderer();
> +        m_recreatingGraphicsContext = false;
> +        return;

It looks like we are going to lose the first composite operation right after recovery of the graphics context. Is this the case? Do we need another call to setRootLayerNeedsDisplay() or similar before returning here? And is it possible that doing so will avoid the need for the reshape hack in reallocateRenderer()?
Comment 5 Alexey Marinichev 2011-03-11 15:42:33 PST
reallocateRenderer invalidates the root layer which will trigger repaint.  I don't think we're losing anything here.  Perhaps I should add a comment about that.
Comment 6 Kenneth Russell 2011-03-11 16:02:12 PST
(In reply to comment #5)
> reallocateRenderer invalidates the root layer which will trigger repaint.  I don't think we're losing anything here.  Perhaps I should add a comment about that.

I think that would be helpful if you wouldn't mind uploading another patch for it.
Comment 7 Kenneth Russell 2011-03-11 16:02:25 PST
(The patch looks good otherwise and I'm happy to r+ it.)
Comment 8 Alexey Marinichev 2011-03-11 16:22:39 PST
Created attachment 85552 [details]
Patch
Comment 9 Kenneth Russell 2011-03-11 16:26:47 PST
Comment on attachment 85552 [details]
Patch

Looks good to me.
Comment 10 WebKit Commit Bot 2011-03-11 16:28:30 PST
Comment on attachment 85552 [details]
Patch

Rejecting attachment 85552 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
src/WebViewImpl.cpp
Hunk #1 succeeded at 1082 (offset 1 line).
Hunk #2 succeeded at 1099 (offset 1 line).
Hunk #3 succeeded at 2414 (offset 14 lines).
Hunk #4 FAILED at 2448.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej
patching file Source/WebKit/chromium/src/WebViewImpl.h
Hunk #1 succeeded at 532 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8141474
Comment 11 Alexey Marinichev 2011-03-11 17:05:41 PST
Created attachment 85557 [details]
Rebased to top of tree.
Comment 12 Kenneth Russell 2011-03-11 19:03:04 PST
Comment on attachment 85557 [details]
Rebased to top of tree.

Let's try again.
Comment 13 WebKit Commit Bot 2011-03-11 20:25:37 PST
Comment on attachment 85557 [details]
Rebased to top of tree.

Rejecting attachment 85557 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2

Last 500 characters of output:
......................................
tables/mozilla_expected_failures/other ..
transforms ....
transforms/2d ............
transforms/3d/general .....
transforms/3d/hit-testing ....
transforms/3d/point-mapping ........
transitions .................
transitions/interrupted-accelerated-transition.html -> failed

Exiting early after 1 failures. 21675 tests run.
516.00s total testing time

21674 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8141544
Comment 14 WebKit Commit Bot 2011-03-11 22:08:19 PST
The commit-queue encountered the following flaky tests while processing attachment 85557 [details]:

transitions/interrupted-accelerated-transition.html bug 56242 (authors: simon.fraser@apple.com and tonyg@chromium.org)
The commit-queue is continuing to process your patch.
Comment 15 Kenneth Russell 2011-03-14 12:20:40 PDT
Comment on attachment 85557 [details]
Rebased to top of tree.

I think that failure is unrelated to this patch. cq+'ing again.
Comment 16 WebKit Commit Bot 2011-03-14 22:56:47 PDT
Comment on attachment 85557 [details]
Rebased to top of tree.

Clearing flags on attachment: 85557

Committed r81113: <http://trac.webkit.org/changeset/81113>
Comment 17 WebKit Commit Bot 2011-03-14 22:56:52 PDT
All reviewed patches have been landed.  Closing bug.