Send notification from GPU process to browser process of offscreen context creation and destruction…

Send notification from GPU process to browser process of offscreen context creation and destruction, and all context lost events. This improves the robustness of blocking of client 3D APIs such as WebGL when GPU resets occur.

Note that this fix depends on the fix for https://bugs.webkit.org/show_bug.cgi?id=103793 .

BUG=112339
TEST=test case from bug


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171910

[Ken Russell (switch to Gerrit), 2012-12-01 02:38:31]

apatrick: please review. Thanks.

content/common/webkitplatformsupport_impl.cc OWNERS => darin

[apatrick, 2012-12-03 20:35:33]

LGTM with a couple of observations.

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
content/browser/gpu/gpu_process_host.cc:715: iter !=
urls_with_live_offscreen_contexts_.end(); ++iter) {
If the GPU process crashes, the GpuProcessHost is automatically destroyed and
this information will be lost. It occurs to me that a serious GPU crash could
cause this to happen without the OnDidLostContext getting through. Maybe you
could do this if the destructor is invoked as well? It's relatively easy to
verify if this is an issue with about:gpucrash.

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
content/browser/gpu/gpu_process_host.cc:716:
GpuDataManagerImpl::GetInstance()->BlockDomainFrom3DAPIs(
I see BlockDomainFrom3DAPIs takes an AutoLock and is probably intended to be
thread safe. I believe the lifetime of the GpuDataManager is forever. Just
making sure you remember this is running on the IO thread.

[Ken Russell (switch to Gerrit), 2012-12-04 05:11:49]

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
content/browser/gpu/gpu_process_host.cc:715: iter !=
urls_with_live_offscreen_contexts_.end(); ++iter) {
On 2012/12/03 20:35:33, apatrick wrote:
> If the GPU process crashes, the GpuProcessHost is automatically destroyed and
> this information will be lost. It occurs to me that a serious GPU crash could
> cause this to happen without the OnDidLostContext getting through. Maybe you
> could do this if the destructor is invoked as well? It's relatively easy to
> verify if this is an issue with about:gpucrash.

Good point. As it happens, the mechanism I added in my earlier patch introducing
the infobar handles the case where the GPU process crashes, but I agree it would
be better to handle it all here. I've refactored the code and now call it both
from here as well as GpuProcessHost's destructor.

https://codereview.chromium.org/11434072/diff/1/content/browser/gpu/gpu_proce...
content/browser/gpu/gpu_process_host.cc:716:
GpuDataManagerImpl::GetInstance()->BlockDomainFrom3DAPIs(
On 2012/12/03 20:35:33, apatrick wrote:
> I see BlockDomainFrom3DAPIs takes an AutoLock and is probably intended to be
> thread safe. I believe the lifetime of the GpuDataManager is forever. Just
> making sure you remember this is running on the IO thread.

Yes, understood, and this is what was intended.

[Ken Russell (switch to Gerrit), 2012-12-04 05:15:06]

Planned final patch. Welcome any further feedback.

[apatrick_chromium, 2012-12-04 21:14:47]

I don't see a change to the GpuProcessHost destructor. Up to you if you want to
treat it as context lost. LGTM.

[Ken Russell (switch to Gerrit), 2012-12-04 21:16:10]

https://codereview.chromium.org/11434072/diff/5001/content/browser/gpu/gpu_pr...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11434072/diff/5001/content/browser/gpu/gpu_pr...
content/browser/gpu/gpu_process_host.cc:435: BlockLiveOffscreenContexts();
This is the change to the GpuProcessHost destructor.

[apatrick_chromium, 2012-12-04 21:24:23]

lgtm

https://codereview.chromium.org/11434072/diff/5001/content/browser/gpu/gpu_pr...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11434072/diff/5001/content/browser/gpu/gpu_pr...
content/browser/gpu/gpu_process_host.cc:435: BlockLiveOffscreenContexts();
On 2012/12/04 21:16:10, kbr wrote:
> This is the change to the GpuProcessHost destructor.

Okay I'm blind then.

[Ken Russell (switch to Gerrit), 2012-12-07 19:32:06]

+palmer for gpu_messages.h OWNERS review

[palmer, 2012-12-07 19:57:59]

Some not-security-relevant style nits, but LGTM from an IPC security
perspective.

https://codereview.chromium.org/11434072/diff/12004/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11434072/diff/12004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:707: // TODO(kbr): would be nice to see
the "offscreen" flag too.
NIT (unrelated to IPC): Better to file a bug and name the bug here.

https://codereview.chromium.org/11434072/diff/12004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:708: TRACE_EVENT2("gpu",
"GpuProcessHost::OnDidLoseContext",
NIT (unrelated to IPC): Not sure this follows the coding guidelines for
whitespace. I get the key/value syntax; maybe put the URL on the same line as
the "url" key.

https://codereview.chromium.org/11434072/diff/12004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:736:
GpuDataManagerImpl::GetInstance()->BlockDomainFrom3DAPIs(
NIT: This might fit on one line?

https://codereview.chromium.org/11434072/diff/12004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:740: void
GpuProcessHost::OnDidDestroyOffscreenContext(
NIT: This might fit on one line?

[commit-bot: I haz the power, 2012-12-07 19:59:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11434072/12004

[commit-bot: I haz the power, 2012-12-07 19:59:44]

Presubmit check for 11434072-12004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common

Presubmit checks took 1.1s to calculate.

[Ken Russell (switch to Gerrit), 2012-12-07 20:03:36]

+brettw for content/common OWNERS review

[brettw, 2012-12-07 20:12:21]

owners lgtm

[brettw, 2012-12-07 20:17:24]

You probably want to strip the reference fragment before inserting or removing
URLs from your multiset in case the ref changes on the page.

[Ken Russell (switch to Gerrit), 2012-12-07 20:23:51]

On 2012/12/07 20:17:24, brettw wrote:
> You probably want to strip the reference fragment before inserting or removing
> URLs from your multiset in case the ref changes on the page.

Thanks for your review. The URL is snapshotted into the context and doesn't
change over time, so I think the code is OK as written.

[commit-bot: I haz the power, 2012-12-07 20:24:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11434072/12004

[commit-bot: I haz the power, 2012-12-07 20:44:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11434072/17002

[commit-bot: I haz the power, 2012-12-08 01:44:08]

Change committed as 171910