Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330283008:
Fix rendering regression for out-of-process iframes (Closed)
|
Created:
6 years, 6 months ago by kenrb Modified:
6 years, 6 months ago CC:
abarth-chromium, blink-reviews, esprehn, jchaffraix+rendering, site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionFix rendering regression for out-of-process iframes
Changes to the compositing path meant the toggle of an iframe to having
a self-painting layer wasn't getting updated.
R=jamesr@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176130
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed condition, as per comment #
Total comments: 3
Messages
Total messages: 14 (0 generated)
James are you able to review this? It's a 2-line patch to fix a regression, but I'd like confirmation that it makes sense.
I'm not sure if this makes sense or not. +cc a number of people who might.
lgtm, what fails with this? Can you mention what tests were failing without this? https://codereview.chromium.org/330283008/diff/1/Source/web/WebLocalFrameImpl... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/330283008/diff/1/Source/web/WebLocalFrameImpl... Source/web/WebLocalFrameImpl.cpp:560: frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); frames will always have layers, you don't need to check for the existence of it here.
We are still working on getting tests for cross-process iframes with --site-per-process, and that fact meant that the regression initially went unnoticed. Out-of-process iframes were previously drawing, and at some point in the last few weeks stopped doing so. I think this is related to the compositing path changes, but it was not trivial to work out an exact CL.
The CQ bit was checked by kenrb@chromium.org
The CQ bit was unchecked by kenrb@chromium.org
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/330283008/20001
Message was sent while issue was closed.
Change committed as 176130
Message was sent while issue was closed.
Yup. Updating the self painting layer bit here is correct. For in-process iframes we do this in RenderLayerCompositor when the frame contents gets composited. For out-of-process, it makes sense to do it here. https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:559: frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); Is the frame guaranteed to have a renderer at this point?
Message was sent while issue was closed.
https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:559: frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); On 2014/06/13 21:29:09, ojan wrote: > Is the frame guaranteed to have a renderer at this point? I was thinking yes, but now that you ask I am not certain. The frame has to be attached to the DOM and have been navigated, which I think means must have had attach() called at some point, but might there possibly be race conditions if it has its renderer later destroyed?
Message was sent while issue was closed.
https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:559: frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); Yeah, I'm not sure. You could put in an ASSERT(frame()->ownerRenderer()) and see if any layout tests or webkit_unit_tests hit it.
Message was sent while issue was closed.
On 2014/06/17 at 16:42:11, ojan wrote: > https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... > File Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... > Source/web/WebLocalFrameImpl.cpp:559: frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); > Yeah, I'm not sure. You could put in an ASSERT(frame()->ownerRenderer()) and see if any layout tests or webkit_unit_tests hit it. In either case, you don't need to block on me here. lgtm whatever you decide is best.
Message was sent while issue was closed.
On 2014/06/17 16:42:11, ojan wrote: > https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... > File Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/330283008/diff/20001/Source/web/WebLocalFrame... > Source/web/WebLocalFrameImpl.cpp:559: > frame()->ownerRenderer()->layer()->updateSelfPaintingLayer(); > Yeah, I'm not sure. You could put in an ASSERT(frame()->ownerRenderer()) and see > if any layout tests or webkit_unit_tests hit it. I put up a CL to add an explicit check. We don't have good test coverage in this area, but a race condition looks very plausible because this signal is coming from another process. We could detach the Frame locally at the same time that its content is rendering elsewhere. |
