Never call GraphicsLayer::paint when fetching painted output from cc.

Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or
paint since the last time a synchronized paint occurred.

BUG=585694
TBR=ajuma@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4d0892b08c7f42a433a278c05498d7c6ddbe8eeb
Cr-Commit-Position: refs/heads/master@{#374864}

[chrishtr, 2016-02-11 01:16:01]

Description was changed from

==========
none

BUG=
==========

to

==========
none

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2016-02-11 01:17:42]

Description was changed from

==========
none

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

BUG=585694
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2016-02-11 01:18:04]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2016-02-11 03:42:45]

This fix looks reasonable since we shouldn't be painting here, so LGTM for this
patch, but I don't understand when or why this became a problem and I think we
need to in order to merge this into any channels (per 545039).

Can you update the change description to explain what's going on in more detail?

This seems like fallout from iframe throttling where a throttled iframe doesn't
update its lifecycle but is then asked to paint? What patch introduced this
crash?

[chrishtr, 2016-02-11 05:27:48]

Description was changed from

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

BUG=585694
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or 
paint since the last time a synchronized paint occurred.

BUG=585694
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2016-02-11 05:28:07]

Description was changed from

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or 
paint since the last time a synchronized paint occurred.

BUG=585694
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or 
paint since the last time a synchronized paint occurred.

BUG=585694
TBR=ajuma@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2016-02-11 05:31:27]

On 2016/02/11 at 03:42:45, pdr wrote:
> This fix looks reasonable since we shouldn't be painting here, so LGTM for
this patch, but I don't understand when or why this became a problem and I think
we need to in order to merge this into any channels (per 545039).

It became a problem a couple of months ago when Xianzhu turned on synchronized
paint. I think my changes to WebViewImpl may have actually
made it worse, by adding more calls to recompute layout if needed during a
frame, but in various resize or frame-pumping callbacks.
Those are just guesses though.

I will find out what exactly caused the layout in the example in crbug 585694
tomorrow.

> 
> Can you update the change description to explain what's going on in more
detail?
> 
> This seems like fallout from iframe throttling where a throttled iframe
doesn't update its lifecycle but is then asked to paint? What patch introduced
this crash?

I don't think it's a fallout of that, throttling is not turned on.

[chrishtr, 2016-02-11 05:31:30]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-02-11 05:32:32]

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

[pdr., 2016-02-11 05:41:21]

On 2016/02/11 at 05:31:27, chrishtr wrote:
> On 2016/02/11 at 03:42:45, pdr wrote:
> > This fix looks reasonable since we shouldn't be painting here, so LGTM for
this patch, but I don't understand when or why this became a problem and I think
we need to in order to merge this into any channels (per 545039).
> 
> It became a problem a couple of months ago when Xianzhu turned on synchronized
paint. I think my changes to WebViewImpl may have actually
> made it worse, by adding more calls to recompute layout if needed during a
frame, but in various resize or frame-pumping callbacks.
> Those are just guesses though.
> 
> I will find out what exactly caused the layout in the example in crbug 585694
tomorrow.
> 
> > 
> > Can you update the change description to explain what's going on in more
detail?
> > 
> > This seems like fallout from iframe throttling where a throttled iframe
doesn't update its lifecycle but is then asked to paint? What patch introduced
this crash?
> 
> I don't think it's a fallout of that, throttling is not turned on.

I think disabling throttling fixes this on linux (curiously, no repro on osx).
Someone turned off my linux box so I can't access it to double-check that
though.

When you get in tomorrow, do you mind hard resetting my linux box... and writing
a followup with a test for this patch :)

[chrishtr, 2016-02-11 05:42:23]

On 2016/02/11 at 05:41:21, pdr wrote:
> On 2016/02/11 at 05:31:27, chrishtr wrote:
> > On 2016/02/11 at 03:42:45, pdr wrote:
> > > This fix looks reasonable since we shouldn't be painting here, so LGTM for
this patch, but I don't understand when or why this became a problem and I think
we need to in order to merge this into any channels (per 545039).
> > 
> > It became a problem a couple of months ago when Xianzhu turned on
synchronized paint. I think my changes to WebViewImpl may have actually
> > made it worse, by adding more calls to recompute layout if needed during a
frame, but in various resize or frame-pumping callbacks.
> > Those are just guesses though.
> > 
> > I will find out what exactly caused the layout in the example in crbug
585694 tomorrow.
> > 
> > > 
> > > Can you update the change description to explain what's going on in more
detail?
> > > 
> > > This seems like fallout from iframe throttling where a throttled iframe
doesn't update its lifecycle but is then asked to paint? What patch introduced
this crash?
> > 
> > I don't think it's a fallout of that, throttling is not turned on.
> 
> I think disabling throttling fixes this on linux (curiously, no repro on osx).
Someone turned off my linux box so I can't access it to double-check that
though.
> 
> When you get in tomorrow, do you mind hard resetting my linux box... and
writing a followup with a test for this patch :)

Yes and yes.

[commit-bot: I haz the power, 2016-02-11 06:23:00]

Committed patchset #4 (id:60001)

[chrishtr, 2016-02-11 19:16:53]

On 2016/02/11 at 05:42:23, chrishtr wrote:
> On 2016/02/11 at 05:41:21, pdr wrote:
> > On 2016/02/11 at 05:31:27, chrishtr wrote:
> > > On 2016/02/11 at 03:42:45, pdr wrote:
> > > > This fix looks reasonable since we shouldn't be painting here, so LGTM
for this patch, but I don't understand when or why this became a problem and I
think we need to in order to merge this into any channels (per 545039).
> > > 
> > > It became a problem a couple of months ago when Xianzhu turned on
synchronized paint. I think my changes to WebViewImpl may have actually
> > > made it worse, by adding more calls to recompute layout if needed during a
frame, but in various resize or frame-pumping callbacks.
> > > Those are just guesses though.
> > > 
> > > I will find out what exactly caused the layout in the example in crbug
585694 tomorrow.
> > > 
> > > > 
> > > > Can you update the change description to explain what's going on in more
detail?
> > > > 
> > > > This seems like fallout from iframe throttling where a throttled iframe
doesn't update its lifecycle but is then asked to paint? What patch introduced
this crash?
> > > 
> > > I don't think it's a fallout of that, throttling is not turned on.
> > 
> > I think disabling throttling fixes this on linux (curiously, no repro on
osx). Someone turned off my linux box so I can't access it to double-check that
though.
> > 
> > When you get in tomorrow, do you mind hard resetting my linux box... and
writing a followup with a test for this patch :)
> 
> Yes and yes.

Update on test: the reason the whole thing is happening is due to
CapturePageText, see my update on crbug 585694. I pinged crbug 585164 so we can
get rid of this situation, more discussion is
happening there.

Also, this does not happen on content shell, testing would be very difficult.
Better to clean up the other call sites directly I think.

[commit-bot: I haz the power, 2016-02-16 22:34:45]

Description was changed from

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or 
paint since the last time a synchronized paint occurred.

BUG=585694
TBR=ajuma@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Never call GraphicsLayer::paint when fetching painted output from cc.

GraphicsLayer::paint may not be a no-op if there happened to be dirty layout or
paint since the last time a synchronized paint occurred.

BUG=585694
TBR=ajuma@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4d0892b08c7f42a433a278c05498d7c6ddbe8eeb
Cr-Commit-Position: refs/heads/master@{#374864}
==========

[commit-bot: I haz the power, 2016-02-16 22:34:46]

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