Remove redundant codepath for webgl api blocking

Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d2ef49671ee9db34151c5a3ca03e18a69560f669
Cr-Commit-Position: refs/heads/master@{#386180}

[no sievers, 2016-03-01 22:47:52]

Description was changed from

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete.

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
==========

to

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete.

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[no sievers, 2016-03-01 22:47:52]

Description was changed from

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete.

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete.

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-03-01 22:49:24]

Description was changed from

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete.

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-03-01 22:49:49]

Description was changed from

==========
Remove unneeded codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-03-01 22:49:49]

sievers@chromium.org changed reviewers:
+ kbr@chromium.org

[no sievers, 2016-03-01 22:50:06]

kbr, ptal

[Ken Russell (switch to Gerrit), 2016-03-02 01:39:29]

Doesn't this CL also have the implication that if the KHR_robustness or
ARB_robustness extensions report a lost context, that the GPU process has to be
torn down in order to cause the lost context notifications?

We should have changed the WEBGL_lose_context extension a while ago to actually
force a lost context at the command buffer level using GL_CHROMIUM_lose_context.
If that change were made, would this CL cause tests like
WebglConformance.conformance_context_context_lost_restored to start failing?

[no sievers, 2016-03-09 21:49:07]

On 2016/03/02 01:39:29, Ken Russell wrote:
> Doesn't this CL also have the implication that if the KHR_robustness or
> ARB_robustness extensions report a lost context, that the GPU process has to
be
> torn down in order to cause the lost context notifications?
> 

It should still get blocked for *all* types of context losses since you added
GpuProcessHost::OnDidLoseContext(), which originates from
GpuCommandBufferStub::OnParseError(). I think nowadays the function name
'OnParseError' is a bit misleading, but this is where all lost context events
other than ChannelError() also get sent to the client.
See GpuScheduler::PutChanged(), where we cause a parse error also for
kLostContext.



> We should have changed the WEBGL_lose_context extension a while ago to
actually
> force a lost context at the command buffer level using
GL_CHROMIUM_lose_context.

But this path is for testing and intentionally does not block the 3d api, right?
Also see WebGLRenderingContextBase.cpp:

if (mode == RealLostContext) 
...
   frame->loader().client()->didLoseWebGLContext(


> If that change were made, would this CL cause tests like
> WebglConformance.conformance_context_context_lost_restored to start failing?

I rebased and submitted a fresh set of tryjobs, let's see what happens with this
patch.

[Ken Russell (switch to Gerrit), 2016-03-10 01:47:08]

On 2016/03/09 21:49:07, sievers wrote:
> On 2016/03/02 01:39:29, Ken Russell wrote:
> > Doesn't this CL also have the implication that if the KHR_robustness or
> > ARB_robustness extensions report a lost context, that the GPU process has to
> be
> > torn down in order to cause the lost context notifications?
> > 
> 
> It should still get blocked for *all* types of context losses since you added
> GpuProcessHost::OnDidLoseContext(), which originates from
> GpuCommandBufferStub::OnParseError(). I think nowadays the function name
> 'OnParseError' is a bit misleading, but this is where all lost context events
> other than ChannelError() also get sent to the client.
> See GpuScheduler::PutChanged(), where we cause a parse error also for
> kLostContext.
> 
> 
> 
> > We should have changed the WEBGL_lose_context extension a while ago to
> actually
> > force a lost context at the command buffer level using
> GL_CHROMIUM_lose_context.
> 
> But this path is for testing and intentionally does not block the 3d api,
right?
> Also see WebGLRenderingContextBase.cpp:
> 
> if (mode == RealLostContext) 
> ...
>    frame->loader().client()->didLoseWebGLContext(

Correct on both counts. Still, with enough bookkeeping we could know that a lost
context via GL_CHROMIUM_lose_context is actually synthetic. The more deeply we
test these code paths, the more robust the browser's lost context recovery will
be.

> > If that change were made, would this CL cause tests like
> > WebglConformance.conformance_context_context_lost_restored to start failing?
> 
> I rebased and submitted a fresh set of tryjobs, let's see what happens with
this
> patch.

Thanks, but I still wonder whether we have some code paths that should be more
strongly tested.

[Ken Russell (switch to Gerrit), 2016-03-14 21:56:19]

On 2016/03/10 01:47:08, Ken Russell wrote:
> On 2016/03/09 21:49:07, sievers wrote:
> > On 2016/03/02 01:39:29, Ken Russell wrote:
> > > Doesn't this CL also have the implication that if the KHR_robustness or
> > > ARB_robustness extensions report a lost context, that the GPU process has
to
> > be
> > > torn down in order to cause the lost context notifications?
> > > 
> > 
> > It should still get blocked for *all* types of context losses since you
added
> > GpuProcessHost::OnDidLoseContext(), which originates from
> > GpuCommandBufferStub::OnParseError(). I think nowadays the function name
> > 'OnParseError' is a bit misleading, but this is where all lost context
events
> > other than ChannelError() also get sent to the client.
> > See GpuScheduler::PutChanged(), where we cause a parse error also for
> > kLostContext.
> > 
> > 
> > 
> > > We should have changed the WEBGL_lose_context extension a while ago to
> > actually
> > > force a lost context at the command buffer level using
> > GL_CHROMIUM_lose_context.
> > 
> > But this path is for testing and intentionally does not block the 3d api,
> right?
> > Also see WebGLRenderingContextBase.cpp:
> > 
> > if (mode == RealLostContext) 
> > ...
> >    frame->loader().client()->didLoseWebGLContext(
> 
> Correct on both counts. Still, with enough bookkeeping we could know that a
lost
> context via GL_CHROMIUM_lose_context is actually synthetic. The more deeply we
> test these code paths, the more robust the browser's lost context recovery
will
> be.
> 
> > > If that change were made, would this CL cause tests like
> > > WebglConformance.conformance_context_context_lost_restored to start
failing?
> > 
> > I rebased and submitted a fresh set of tryjobs, let's see what happens with
> this
> > patch.
> 
> Thanks, but I still wonder whether we have some code paths that should be more
> strongly tested.

I tested this patch with a couple of the TDR stress tests on Windows, in
particular
https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html
. Unfortunately, it looks like it destabilizes the browser; I've seen the
browser process both hang and crash, behaviors that don't happen on current
Canary. I'm rebuilding now to see whether it's the GYP_DEFINE dcheck_always_on=1
that's causing this.

[no sievers, 2016-03-15 23:20:19]

On 2016/03/14 21:56:19, Ken Russell wrote:
> On 2016/03/10 01:47:08, Ken Russell wrote:
> > On 2016/03/09 21:49:07, sievers wrote:
> > > On 2016/03/02 01:39:29, Ken Russell wrote:
> > > > Doesn't this CL also have the implication that if the KHR_robustness or
> > > > ARB_robustness extensions report a lost context, that the GPU process
has
> to
> > > be
> > > > torn down in order to cause the lost context notifications?
> > > > 
> > > 
> > > It should still get blocked for *all* types of context losses since you
> added
> > > GpuProcessHost::OnDidLoseContext(), which originates from
> > > GpuCommandBufferStub::OnParseError(). I think nowadays the function name
> > > 'OnParseError' is a bit misleading, but this is where all lost context
> events
> > > other than ChannelError() also get sent to the client.
> > > See GpuScheduler::PutChanged(), where we cause a parse error also for
> > > kLostContext.
> > > 
> > > 
> > > 
> > > > We should have changed the WEBGL_lose_context extension a while ago to
> > > actually
> > > > force a lost context at the command buffer level using
> > > GL_CHROMIUM_lose_context.
> > > 
> > > But this path is for testing and intentionally does not block the 3d api,
> > right?
> > > Also see WebGLRenderingContextBase.cpp:
> > > 
> > > if (mode == RealLostContext) 
> > > ...
> > >    frame->loader().client()->didLoseWebGLContext(
> > 
> > Correct on both counts. Still, with enough bookkeeping we could know that a
> lost
> > context via GL_CHROMIUM_lose_context is actually synthetic. The more deeply
we
> > test these code paths, the more robust the browser's lost context recovery
> will
> > be.
> > 
> > > > If that change were made, would this CL cause tests like
> > > > WebglConformance.conformance_context_context_lost_restored to start
> failing?
> > > 
> > > I rebased and submitted a fresh set of tryjobs, let's see what happens
with
> > this
> > > patch.
> > 
> > Thanks, but I still wonder whether we have some code paths that should be
more
> > strongly tested.
> 
> I tested this patch with a couple of the TDR stress tests on Windows, in
> particular
>
https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html
> . Unfortunately, it looks like it destabilizes the browser; I've seen the
> browser process both hang and crash, behaviors that don't happen on current
> Canary. I'm rebuilding now to see whether it's the GYP_DEFINE
dcheck_always_on=1
> that's causing this.

Thanks for testing this! Let me know if you find out more.

I definitely tripped a couple DCHECKs in the renderer when testing context lost
recovery (but not necessarily in the browser).

[Ken Russell (switch to Gerrit), 2016-03-16 00:51:19]

On 2016/03/15 23:20:19, sievers (slow) wrote:
> On 2016/03/14 21:56:19, Ken Russell wrote:
> > On 2016/03/10 01:47:08, Ken Russell wrote:
> > > On 2016/03/09 21:49:07, sievers wrote:
> > > > On 2016/03/02 01:39:29, Ken Russell wrote:
> > > > > Doesn't this CL also have the implication that if the KHR_robustness
or
> > > > > ARB_robustness extensions report a lost context, that the GPU process
> has
> > to
> > > > be
> > > > > torn down in order to cause the lost context notifications?
> > > > > 
> > > > 
> > > > It should still get blocked for *all* types of context losses since you
> > added
> > > > GpuProcessHost::OnDidLoseContext(), which originates from
> > > > GpuCommandBufferStub::OnParseError(). I think nowadays the function name
> > > > 'OnParseError' is a bit misleading, but this is where all lost context
> > events
> > > > other than ChannelError() also get sent to the client.
> > > > See GpuScheduler::PutChanged(), where we cause a parse error also for
> > > > kLostContext.
> > > > 
> > > > 
> > > > 
> > > > > We should have changed the WEBGL_lose_context extension a while ago to
> > > > actually
> > > > > force a lost context at the command buffer level using
> > > > GL_CHROMIUM_lose_context.
> > > > 
> > > > But this path is for testing and intentionally does not block the 3d
api,
> > > right?
> > > > Also see WebGLRenderingContextBase.cpp:
> > > > 
> > > > if (mode == RealLostContext) 
> > > > ...
> > > >    frame->loader().client()->didLoseWebGLContext(
> > > 
> > > Correct on both counts. Still, with enough bookkeeping we could know that
a
> > lost
> > > context via GL_CHROMIUM_lose_context is actually synthetic. The more
deeply
> we
> > > test these code paths, the more robust the browser's lost context recovery
> > will
> > > be.
> > > 
> > > > > If that change were made, would this CL cause tests like
> > > > > WebglConformance.conformance_context_context_lost_restored to start
> > failing?
> > > > 
> > > > I rebased and submitted a fresh set of tryjobs, let's see what happens
> with
> > > this
> > > > patch.
> > > 
> > > Thanks, but I still wonder whether we have some code paths that should be
> more
> > > strongly tested.
> > 
> > I tested this patch with a couple of the TDR stress tests on Windows, in
> > particular
> >
>
https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html
> > . Unfortunately, it looks like it destabilizes the browser; I've seen the
> > browser process both hang and crash, behaviors that don't happen on current
> > Canary. I'm rebuilding now to see whether it's the GYP_DEFINE
> dcheck_always_on=1
> > that's causing this.
> 
> Thanks for testing this! Let me know if you find out more.
> 
> I definitely tripped a couple DCHECKs in the renderer when testing context
lost
> recovery (but not necessarily in the browser).

Without dcheck_always_on=1, your patch doesn't seem to trigger the "Rats!"
infobar any more on Windows. This indicates that the DoS defense mechanism isn't
working.

Do you have a Windows workstation on which to test your patch?

[no sievers, 2016-03-18 21:54:36]

On 2016/03/16 00:51:19, Ken Russell wrote:
> On 2016/03/15 23:20:19, sievers (slow) wrote:
> > On 2016/03/14 21:56:19, Ken Russell wrote:
> > > On 2016/03/10 01:47:08, Ken Russell wrote:
> > > > On 2016/03/09 21:49:07, sievers wrote:
> > > > > On 2016/03/02 01:39:29, Ken Russell wrote:
> > > > > > Doesn't this CL also have the implication that if the KHR_robustness
> or
> > > > > > ARB_robustness extensions report a lost context, that the GPU
process
> > has
> > > to
> > > > > be
> > > > > > torn down in order to cause the lost context notifications?
> > > > > > 
> > > > > 
> > > > > It should still get blocked for *all* types of context losses since
you
> > > added
> > > > > GpuProcessHost::OnDidLoseContext(), which originates from
> > > > > GpuCommandBufferStub::OnParseError(). I think nowadays the function
name
> > > > > 'OnParseError' is a bit misleading, but this is where all lost context
> > > events
> > > > > other than ChannelError() also get sent to the client.
> > > > > See GpuScheduler::PutChanged(), where we cause a parse error also for
> > > > > kLostContext.
> > > > > 
> > > > > 
> > > > > 
> > > > > > We should have changed the WEBGL_lose_context extension a while ago
to
> > > > > actually
> > > > > > force a lost context at the command buffer level using
> > > > > GL_CHROMIUM_lose_context.
> > > > > 
> > > > > But this path is for testing and intentionally does not block the 3d
> api,
> > > > right?
> > > > > Also see WebGLRenderingContextBase.cpp:
> > > > > 
> > > > > if (mode == RealLostContext) 
> > > > > ...
> > > > >    frame->loader().client()->didLoseWebGLContext(
> > > > 
> > > > Correct on both counts. Still, with enough bookkeeping we could know
that
> a
> > > lost
> > > > context via GL_CHROMIUM_lose_context is actually synthetic. The more
> deeply
> > we
> > > > test these code paths, the more robust the browser's lost context
recovery
> > > will
> > > > be.
> > > > 
> > > > > > If that change were made, would this CL cause tests like
> > > > > > WebglConformance.conformance_context_context_lost_restored to start
> > > failing?
> > > > > 
> > > > > I rebased and submitted a fresh set of tryjobs, let's see what happens
> > with
> > > > this
> > > > > patch.
> > > > 
> > > > Thanks, but I still wonder whether we have some code paths that should
be
> > more
> > > > strongly tested.
> > > 
> > > I tested this patch with a couple of the TDR stress tests on Windows, in
> > > particular
> > >
> >
>
https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html
> > > . Unfortunately, it looks like it destabilizes the browser; I've seen the
> > > browser process both hang and crash, behaviors that don't happen on
current
> > > Canary. I'm rebuilding now to see whether it's the GYP_DEFINE
> > dcheck_always_on=1
> > > that's causing this.
> > 
> > Thanks for testing this! Let me know if you find out more.
> > 
> > I definitely tripped a couple DCHECKs in the renderer when testing context
> lost
> > recovery (but not necessarily in the browser).
> 
> Without dcheck_always_on=1, your patch doesn't seem to trigger the "Rats!"
> infobar any more on Windows. This indicates that the DoS defense mechanism
isn't
> working.
> 
> Do you have a Windows workstation on which to test your patch?

I don't have a Windows workstation but I tried the same shader on Linux and I
see the infobar.
It goes through GpuProcessHost::OnDidLoseContext() repeatedly (for the different
contexts) and sees both 'guilty or unknown' context and the onscreen one getting
lost and blocks the domain.

Did it sometimes not show the infobar for you, or never?

Because I guess there might be a race in the ctx lost signaling between
GPU->browser blocking the domain and GPU->renderer->browser recreating the
context. Seems very corner-case'ish though.
And it would have to be blocked the second time for sure.

[Ken Russell (switch to Gerrit), 2016-03-19 00:52:05]

On 2016/03/18 21:54:36, sievers wrote:
> On 2016/03/16 00:51:19, Ken Russell wrote:
> > On 2016/03/15 23:20:19, sievers (slow) wrote:
> > > On 2016/03/14 21:56:19, Ken Russell wrote:
> > > > On 2016/03/10 01:47:08, Ken Russell wrote:
> > > > > On 2016/03/09 21:49:07, sievers wrote:
> > > > > > On 2016/03/02 01:39:29, Ken Russell wrote:
> > > > > > > Doesn't this CL also have the implication that if the
KHR_robustness
> > or
> > > > > > > ARB_robustness extensions report a lost context, that the GPU
> process
> > > has
> > > > to
> > > > > > be
> > > > > > > torn down in order to cause the lost context notifications?
> > > > > > > 
> > > > > > 
> > > > > > It should still get blocked for *all* types of context losses since
> you
> > > > added
> > > > > > GpuProcessHost::OnDidLoseContext(), which originates from
> > > > > > GpuCommandBufferStub::OnParseError(). I think nowadays the function
> name
> > > > > > 'OnParseError' is a bit misleading, but this is where all lost
context
> > > > events
> > > > > > other than ChannelError() also get sent to the client.
> > > > > > See GpuScheduler::PutChanged(), where we cause a parse error also
for
> > > > > > kLostContext.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > We should have changed the WEBGL_lose_context extension a while
ago
> to
> > > > > > actually
> > > > > > > force a lost context at the command buffer level using
> > > > > > GL_CHROMIUM_lose_context.
> > > > > > 
> > > > > > But this path is for testing and intentionally does not block the 3d
> > api,
> > > > > right?
> > > > > > Also see WebGLRenderingContextBase.cpp:
> > > > > > 
> > > > > > if (mode == RealLostContext) 
> > > > > > ...
> > > > > >    frame->loader().client()->didLoseWebGLContext(
> > > > > 
> > > > > Correct on both counts. Still, with enough bookkeeping we could know
> that
> > a
> > > > lost
> > > > > context via GL_CHROMIUM_lose_context is actually synthetic. The more
> > deeply
> > > we
> > > > > test these code paths, the more robust the browser's lost context
> recovery
> > > > will
> > > > > be.
> > > > > 
> > > > > > > If that change were made, would this CL cause tests like
> > > > > > > WebglConformance.conformance_context_context_lost_restored to
start
> > > > failing?
> > > > > > 
> > > > > > I rebased and submitted a fresh set of tryjobs, let's see what
happens
> > > with
> > > > > this
> > > > > > patch.
> > > > > 
> > > > > Thanks, but I still wonder whether we have some code paths that should
> be
> > > more
> > > > > strongly tested.
> > > > 
> > > > I tested this patch with a couple of the TDR stress tests on Windows, in
> > > > particular
> > > >
> > >
> >
>
https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html
> > > > . Unfortunately, it looks like it destabilizes the browser; I've seen
the
> > > > browser process both hang and crash, behaviors that don't happen on
> current
> > > > Canary. I'm rebuilding now to see whether it's the GYP_DEFINE
> > > dcheck_always_on=1
> > > > that's causing this.
> > > 
> > > Thanks for testing this! Let me know if you find out more.
> > > 
> > > I definitely tripped a couple DCHECKs in the renderer when testing context
> > lost
> > > recovery (but not necessarily in the browser).
> > 
> > Without dcheck_always_on=1, your patch doesn't seem to trigger the "Rats!"
> > infobar any more on Windows. This indicates that the DoS defense mechanism
> isn't
> > working.
> > 
> > Do you have a Windows workstation on which to test your patch?
> 
> I don't have a Windows workstation but I tried the same shader on Linux and I
> see the infobar.
> It goes through GpuProcessHost::OnDidLoseContext() repeatedly (for the
different
> contexts) and sees both 'guilty or unknown' context and the onscreen one
getting
> lost and blocks the domain.
> 
> Did it sometimes not show the infobar for you, or never?
> 
> Because I guess there might be a race in the ctx lost signaling between
> GPU->browser blocking the domain and GPU->renderer->browser recreating the
> context. Seems very corner-case'ish though.
> And it would have to be blocked the second time for sure.

After I removed dcheck_always_on=1 I didn't see the infobar at all on Windows
for that particular test case, when alternated with other tests which attempted
to fetch WebGL contexts. If there is a race now, as opposed to a general
correctness problem with context lost reporting, it seems pretty reliable.

[no sievers, 2016-03-21 17:37:56]

I wonder if it has to do with the fact that windows uses the
'exit_on_context_lost' workaround (see driver bug json file).

We still send the per-context notifications (to browser and renderer) from the
same function, but there is one subtle difference:
- in the renderer 'channel lost' is treated as GL_UNKNOWN_CONTEXT_RESET_KHR (see
GLES2Implementation::GetGraphicsResetStatusKHR())
- while the browser would see the original context lost reason for the specific
context (guilty, innocent, unknown, etc.)

Now also consider my other patch where I made the change to not block all
offscreen contexts for 'normal' GPU process shutdown, which is the third path
that blocks 3d apis, but would now skip it for intentional 'normal' shutdowns
('exit_on_context_lost').

I tried forcing this on Linux, but it's still falling through the 'guilty ||
innocent' path in the browser.
But there could be subtle differences on Windows (in signaling related to the
lost pipe and pending messages, or even what context we discover the reset on
first).

I'm wondering if my other patch should be restricted to 'Android' after all,
and/or also maybe if we should mark a context that forces a
'exit_on_context_lost' shutdown as 'unknown' (to make sure it's not marked as
'innocent').

[no sievers, 2016-03-21 18:28:50]

On 2016/03/21 17:37:56, sievers wrote:
> I wonder if it has to do with the fact that windows uses the
> 'exit_on_context_lost' workaround (see driver bug json file).
> 
> We still send the per-context notifications (to browser and renderer) from the
> same function, but there is one subtle difference:
> - in the renderer 'channel lost' is treated as GL_UNKNOWN_CONTEXT_RESET_KHR
(see
> GLES2Implementation::GetGraphicsResetStatusKHR())
> - while the browser would see the original context lost reason for the
specific
> context (guilty, innocent, unknown, etc.)
> 
> Now also consider my other patch where I made the change to not block all
> offscreen contexts for 'normal' GPU process shutdown, which is the third path
> that blocks 3d apis, but would now skip it for intentional 'normal' shutdowns
> ('exit_on_context_lost').
> 
> I tried forcing this on Linux, but it's still falling through the 'guilty ||
> innocent' path in the browser.
> But there could be subtle differences on Windows (in signaling related to the
> lost pipe and pending messages, or even what context we discover the reset on
> first).
> 
> I'm wondering if my other patch should be restricted to 'Android' after all,
> and/or also maybe if we should mark a context that forces a
> 'exit_on_context_lost' shutdown as 'unknown' (to make sure it's not marked as
> 'innocent').

see ps4 (vs. ps3) for what I'm thinking specifically

[no sievers, 2016-03-21 18:31:15]

https://codereview.chromium.org/1747283003/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1747283003/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:732: // shutdown, so make sure it
treats it seriously enough wrt 3D api blocking.
Though there's probably a cleaner way to just handle this in the browser-side
logic.

[Ken Russell (switch to Gerrit), 2016-03-21 23:53:01]

Excellent. Thank you for persisting with this fix. LGTM

https://codereview.chromium.org/1747283003/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1747283003/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:732: // shutdown, so make sure it
treats it seriously enough wrt 3D api blocking.
On 2016/03/21 18:31:15, sievers wrote:
> Though there's probably a cleaner way to just handle this in the browser-side
> logic.

Very nice. Thank you for tracking down the need to add this. With this change,
the "Rats!" infobar is showing up reliably, and properly, on Windows. Tested by
doing the following steps repeatedly:

Visiting:

https://www.khronos.org/registry/webgl/sdk/tests/extra/slow-shader-example.html

waiting for "Rats!", then visiting:

https://www.khronos.org/registry/webgl/sdk/tests/extra/canvas-compositing-tes...

and alternately ignoring and reloading the page via the infobar.

[no sievers, 2016-03-22 00:13:00]

On 2016/03/21 23:53:01, Ken Russell wrote:
> Tested by
> doing the following steps repeatedly:
>  [...]

Thanks for bringing this up and testing this. Am glad we found this subtle
issue.

I'll try to move the 'exit_on_context_lost'-related changes from
GpuCommandBuffer stub to the browser (where it's decoding to treat the crash as
normal or not) before landing. It should be equivalent to check for the
workaround there.

[Ken Russell (switch to Gerrit), 2016-03-22 00:15:18]

On 2016/03/22 00:13:00, sievers wrote:
> On 2016/03/21 23:53:01, Ken Russell wrote:
> > Tested by
> > doing the following steps repeatedly:
> >  [...]
> 
> Thanks for bringing this up and testing this. Am glad we found this subtle
> issue.
> 
> I'll try to move the 'exit_on_context_lost'-related changes from
> GpuCommandBuffer stub to the browser (where it's decoding to treat the crash
as
> normal or not) before landing. It should be equivalent to check for the
> workaround there.

Let me know when you have that change ready and I'll re-test it for you.

[no sievers, 2016-03-22 20:03:18]

On 2016/03/22 00:15:18, Ken Russell wrote:
> On 2016/03/22 00:13:00, sievers wrote:
> > On 2016/03/21 23:53:01, Ken Russell wrote:
> > > Tested by
> > > doing the following steps repeatedly:
> > >  [...]
> > 
> > Thanks for bringing this up and testing this. Am glad we found this subtle
> > issue.
> > 
> > I'll try to move the 'exit_on_context_lost'-related changes from
> > GpuCommandBuffer stub to the browser (where it's decoding to treat the crash
> as
> > normal or not) before landing. It should be equivalent to check for the
> > workaround there.
> 
> Let me know when you have that change ready and I'll re-test it for you.

Cool, I've reverted the changes to GpuCommandBufferStub in ps5.

I think it should work as before since we would detect all process crashes in
the browser and would treat them as serious.
(On Android the logic in GpuProcessHost is valid, since the exit status is never
'normal' when the browser was in the foreground.)

To remove the #ifdef-Android I think we'd want to communicate that the GPU
process was shut down because of a serious error (and the exit_on_context_lost
workaround) to the browser, which I'll leave for another patch.

[Ken Russell (switch to Gerrit), 2016-03-28 22:00:38]

The most recent patch set doesn't bring up the "Rats!" infobar on Windows as it
should, when running stress tests like slow-shader-example.html from the Khronos
WebGL repo.

[Ken Russell (switch to Gerrit), 2016-03-28 22:09:59]

On 2016/03/28 22:00:38, Ken Russell wrote:
> The most recent patch set doesn't bring up the "Rats!" infobar on Windows as
it
> should, when running stress tests like slow-shader-example.html from the
Khronos
> WebGL repo.

I confirmed that that's a behavioral change of the most recent patch set.
Undoing it causes the infobar to be brought up as expected on ToT.

[Ken Russell (switch to Gerrit), 2016-03-28 22:12:21]

On 2016/03/28 22:09:59, Ken Russell wrote:
> On 2016/03/28 22:00:38, Ken Russell wrote:
> > The most recent patch set doesn't bring up the "Rats!" infobar on Windows as
> it
> > should, when running stress tests like slow-shader-example.html from the
> Khronos
> > WebGL repo.
> 
> I confirmed that that's a behavioral change of the most recent patch set.
> Undoing it causes the infobar to be brought up as expected on ToT.

Note: it's a little worrying that this regression isn't caught by the
context_lost tests. I'm not sure why the webkit_tests step in the
win_chromium_rel_ng run
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
was aborted, but the results of context_lost aren't showing up for that run.

[no sievers, 2016-04-01 22:26:39]

Sorry for the delay. Patch set 8 adds a bunch of logging around the domain
blocking and lost context signaling.

[no sievers, 2016-04-04 19:20:46]

On 2016/04/01 22:26:39, sievers wrote:
> Sorry for the delay. Patch set 8 adds a bunch of logging around the domain
> blocking and lost context signaling.

So I have a suspicion:
GpuProcessHost::urls_with_live_offscreen_contexts_ is what's used for the
blocking webgl when the GPU process is lost.

However, look at https://codereview.chromium.org/1678183003 which means that we
actually now tear down things properly which causes
GpuCommandBufferStub::Destroy() and removing all of them from
|urls_with_live_offscreen_contexts_| in the browser which probably races with
seeing the channel gone.

Maybe when we are about to intentionally shut down the GPU process we just skip
sending these notifcations intentionally. wdyt?

[Ken Russell (switch to Gerrit), 2016-04-04 23:00:14]

On 2016/04/04 19:20:46, sievers wrote:
> On 2016/04/01 22:26:39, sievers wrote:
> > Sorry for the delay. Patch set 8 adds a bunch of logging around the domain
> > blocking and lost context signaling.
> 
> So I have a suspicion:
> GpuProcessHost::urls_with_live_offscreen_contexts_ is what's used for the
> blocking webgl when the GPU process is lost.
> 
> However, look at https://codereview.chromium.org/1678183003 which means that
we
> actually now tear down things properly which causes
> GpuCommandBufferStub::Destroy() and removing all of them from
> |urls_with_live_offscreen_contexts_| in the browser which probably races with
> seeing the channel gone.
> 
> Maybe when we are about to intentionally shut down the GPU process we just
skip
> sending these notifcations intentionally. wdyt?

It's possible.

Do you have a Windows machine on which to test this? My Windows workspace is in
an intermediate state right now.

[no sievers, 2016-04-04 23:03:53]

On 2016/04/04 23:00:14, Ken Russell wrote:
> On 2016/04/04 19:20:46, sievers wrote:
> > On 2016/04/01 22:26:39, sievers wrote:
> > > Sorry for the delay. Patch set 8 adds a bunch of logging around the domain
> > > blocking and lost context signaling.
> > 
> > So I have a suspicion:
> > GpuProcessHost::urls_with_live_offscreen_contexts_ is what's used for the
> > blocking webgl when the GPU process is lost.
> > 
> > However, look at https://codereview.chromium.org/1678183003 which means that
> we
> > actually now tear down things properly which causes
> > GpuCommandBufferStub::Destroy() and removing all of them from
> > |urls_with_live_offscreen_contexts_| in the browser which probably races
with
> > seeing the channel gone.
> > 
> > Maybe when we are about to intentionally shut down the GPU process we just
> skip
> > sending these notifcations intentionally. wdyt?
> 
> It's possible.
> 
> Do you have a Windows machine on which to test this? My Windows workspace is
in
> an intermediate state right now.

Yes, ericrk@ here is volunteering his machine.

[Ken Russell (switch to Gerrit), 2016-04-04 23:04:38]

On 2016/04/04 23:03:53, sievers wrote:
> On 2016/04/04 23:00:14, Ken Russell wrote:
> > On 2016/04/04 19:20:46, sievers wrote:
> > > On 2016/04/01 22:26:39, sievers wrote:
> > > > Sorry for the delay. Patch set 8 adds a bunch of logging around the
domain
> > > > blocking and lost context signaling.
> > > 
> > > So I have a suspicion:
> > > GpuProcessHost::urls_with_live_offscreen_contexts_ is what's used for the
> > > blocking webgl when the GPU process is lost.
> > > 
> > > However, look at https://codereview.chromium.org/1678183003 which means
that
> > we
> > > actually now tear down things properly which causes
> > > GpuCommandBufferStub::Destroy() and removing all of them from
> > > |urls_with_live_offscreen_contexts_| in the browser which probably races
> with
> > > seeing the channel gone.
> > > 
> > > Maybe when we are about to intentionally shut down the GPU process we just
> > skip
> > > sending these notifcations intentionally. wdyt?
> > 
> > It's possible.
> > 
> > Do you have a Windows machine on which to test this? My Windows workspace is
> in
> > an intermediate state right now.
> 
> Yes, ericrk@ here is volunteering his machine.

Thanks, please let me know what you find in your local testing.

[no sievers, 2016-04-07 20:23:50]

On 2016/04/04 23:04:38, Ken Russell wrote:
> Thanks, please let me know what you find in your local testing.


Ok I just tried this out and was able to confirm my theory.

I verified that - on Windows at least - we generally in the browser
- see GpuHostMsg_DidDestroyOffscreenContext first for all the offscreen contexts
(because we tear down things properly instead of exit() now)
- when ~GpuProcessHost happens from the channel disconnect we are tracking 0
offscreen contexts

Combined with the fact that we only see GpuHostMsg_DidLoseContext for the very
first context that causes the shutdown and that often on Windows seems to be 
not recognized as 'guilty || unknown' this was bypassing the blocking.

I fixed it in patch set 12 by not sending GpuHostMsg_DidDestroyOffscreenContext
when we are currently intentionally shutting down, and that fixes it.

[no sievers, 2016-04-07 20:26:23]

>  that often on Windows seems to be not recognized as 'guilty || unknown'

In case you're wondering: it's MakeCurrent() failing and we therefore can't
query getGraphicsResetStatus() for that context.. well since we weren't able to
make it current.

[no sievers, 2016-04-07 22:03:04]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process for
'exit_on_context_lost'.

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-07 22:06:42]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process for
'exit_on_context_lost'.

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/ i.e. need to limit to Android
after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-07 22:07:01]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/ i.e. need to limit to Android
after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-07 22:07:20]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete, see
GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Ken Russell (switch to Gerrit), 2016-04-08 01:16:41]

Tested patch set 12 on Windows and it reliably brings up the infobar for stress
tests that cause TDRs. LGTM based on that -- haven't reviewed the latest code in
detail yet.

[Ken Russell (switch to Gerrit), 2016-04-08 01:21:31]

Reviewed the latest code -- looks good. Thanks again for persevering with this
cleanup.

[no sievers, 2016-04-08 01:32:59]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-04-08 01:33:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747283003/200002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747283003/200002

[commit-bot: I haz the power, 2016-04-08 01:56:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 01:56:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[esprehn, 2016-04-08 02:16:33]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BlockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Ken Russell (switch to Gerrit), 2016-04-08 02:16:34]

kbr@chromium.org changed reviewers:
+ esprehn@chromium.org

[Ken Russell (switch to Gerrit), 2016-04-08 02:16:35]

+esprehn for OWNERS review of WebFrameClient.h

[esprehn, 2016-04-08 02:17:16]

lgtm

https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.h:150: bool IsExitingForContextLost() {
why isn't this override or hacker_case?

[Ken Russell (switch to Gerrit), 2016-04-08 02:20:25]

On 2016/04/08 02:17:16, esprehn wrote:
> lgtm
> 
>
https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
> File content/common/gpu/gpu_channel_manager.h (right):
> 
>
https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
> content/common/gpu/gpu_channel_manager.h:150: bool IsExitingForContextLost() {
> why isn't this override or hacker_case?

Fair question -- since this fix is desired for the branch point let me re-CQ it
now and ask Daniel to answer tomorrow. Thanks for your quick review.

[Ken Russell (switch to Gerrit), 2016-04-08 02:20:37]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-04-08 02:21:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747283003/200002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747283003/200002

[commit-bot: I haz the power, 2016-04-08 02:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 02:36:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[no sievers, 2016-04-08 17:09:15]

sievers@chromium.org changed reviewers:
+ nasko@chromium.org

[no sievers, 2016-04-08 17:12:36]

On 2016/04/08 02:17:16, esprehn wrote:
> lgtm
> 
>
https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
> File content/common/gpu/gpu_channel_manager.h (right):
> 
>
https://codereview.chromium.org/1747283003/diff/200002/content/common/gpu/gpu...
> content/common/gpu/gpu_channel_manager.h:150: bool IsExitingForContextLost() {
> why isn't this override or hacker_case?

Done.

[nasko, 2016-04-08 17:16:56]

IPC LGTM.

[no sievers, 2016-04-08 17:26:40]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2016-04-08 17:26:40]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
esprehn@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1747283003/#ps250001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-08 17:27:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747283003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747283003/250001

[no sievers, 2016-04-08 19:49:22]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-08 19:49:37]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2016-04-08 19:49:38]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2016-04-08 19:50:19]

The CQ bit was unchecked by sievers@chromium.org

[commit-bot: I haz the power, 2016-04-08 19:50:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747283003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747283003/250001

[no sievers, 2016-04-08 19:51:00]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2016-04-08 19:51:22]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-08 19:58:07]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[no sievers, 2016-04-08 19:58:16]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-04-08 19:58:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747283003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747283003/250001

[commit-bot: I haz the power, 2016-04-08 20:05:46]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-08 20:05:48]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-04-08 20:07:09]

Description was changed from

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove redundant codepath for webgl api blocking

https://chromiumcodereview.appspot.com/11434072
handles this more generically and makes the original codepath
from https://chromiumcodereview.appspot.com/11378008 obsolete,
 see GpuProcessHost::OnDidLoseContext().

The motivation for removing this is that it also makes it harder
to special-case context losses from the OOM killer on Android
if the renderer tries to invoke 3D api blocking (rather than
the GPU process itself).

There was a subtle interaction wrt the 'exit_on_context' lost
logic which manifested itself on Windows. To still raise
the infobar reliably, this patch also forces us to go
through the BLockLiveOffscreenContexts() logic in ~GpuProcessHost
as it was intended, even if we cleanly tear down the GPU process
for 'exit_on_context_lost'. This required
- blocking offscreen contexts regardless of the
 status code (TODO crbug.com/598400)
(see https://codereview.chromium.org/1749263003/
 i.e. need to limit to Android after all)
- knowing about live offscreen contexts even when we shutdown cleanly
(see https://codereview.chromium.org/1678183003)

NOTRY=True (win webkit_tests flaky and slow as usual)
BUG=586905
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d2ef49671ee9db34151c5a3ca03e18a69560f669
Cr-Commit-Position: refs/heads/master@{#386180}
==========

[commit-bot: I haz the power, 2016-04-08 20:07:11]

Patchset 14 (id:??) landed as
https://crrev.com/d2ef49671ee9db34151c5a3ca03e18a69560f669
Cr-Commit-Position: refs/heads/master@{#386180}