Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1906803002:
Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() (Closed)
|
Created:
4 years, 8 months ago by kojii Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout()
When we're in layout, markContainerChainForLayout() is marking a
descendant as needing layout with the intention of visiting it during
this layout. We shouldn't be scheduling it to be laid out later.
Also, schedduleRelayout() must not be called while iterating
FrameView::m_layoutSubtreeRootList.
BUG=602908, 589773
Committed: https://crrev.com/1a0aa2dff16abbd9bc7f906b4d807766668bde17
Cr-Commit-Position: refs/heads/master@{#389691}
Patch Set 1 #Patch Set 2 : RELEASE_ASSERT to FrameView #
Total comments: 2
Patch Set 3 : leviw/cbiesinger reviews #
Total comments: 1
Patch Set 4 : Fix typo "scheddule" #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== WIP: schdule-relayout-auto BUG=602908 ========== to ========== Prevent markContainerChainForLayout not to scheduleRelayout() if isInPerformLayout() BUG=602908 ==========
Description was changed from ========== Prevent markContainerChainForLayout not to scheduleRelayout() if isInPerformLayout() BUG=602908 ========== to ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() BUG=602908 ==========
kojii@chromium.org changed reviewers: + cbiesinger@chromium.org, leviw@chromium.org
WDYT about forcibly turn scheduleRelayout() off if isInPerfomLayout()? I put the assert and tried to fix where it hits the assertion in crrev.com/1908763003, but "100 failures" wasn't correct. Bots stopped after 100 errors, and as I fix, more errors appear. Some are easy and fixed in the CL above, but some are hard, low in the stack, such as LayoutTable::recalcCells() or LayoutSVGResourceContainer::markForLayoutAndParentResourceInvalidation(). They can be called outside of layout too, e.g., DOM mutations or hit testing, so we need to pass an argument down to them, but oftentimes it goes through virtual functions and it's hard. So I started to wonder whether it's valuable enough to make inLayout/not explicit to pass down to this function or not, made it automatic and it looks like all tests pass. If this regressed somehow and we found cases where we really need to rescheduleRelayout() during isInPerformLayout() but not while iterating m_layoutSubtreeRootList, we could support that by adding an enum value. https://codereview.chromium.org/1906803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1906803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1784: RELEASE_ASSERT(!isInPerformLayout()); The critical error is to modify m_layoutSubtreeRootList (such as clearLayoutSubtreeRootsAndMarkContainingBlocks()) while we're iterating it in: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... but we need to clear it after the iteration, so we can't assert in DepthOrderedLayoutObjectList::clear(). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... RELEASE_ASSERT here is the lowest place where we can turn security issues to crashes.
I think this is totally reasonable. LGTM. https://codereview.chromium.org/1906803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1906803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:806: if (scheduleRelayout && !frameView()->isInPerformLayout()) This is probably worthy of a comment.
lgtm. I agree with Levi's suggestion of a comment.
On 2016/04/22 18:01:29, cbiesinger wrote: > lgtm. I agree with Levi's suggestion of a comment. Actually, hold on- scheduleRelayout does have a second effect in this function, for an early loop exit. I think we want that effect in this case too? Maybe the function should set scheduleRelayout to false if isInPerformLayout() at the top?
Description was changed from ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() BUG=602908 ========== to ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, we're marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908 ==========
On 2016/04/22 at 18:03:21, cbiesinger wrote: > On 2016/04/22 18:01:29, cbiesinger wrote: > > lgtm. I agree with Levi's suggestion of a comment. > > Actually, hold on- > > scheduleRelayout does have a second effect in this function, for an early loop exit. I think we want that effect in this case too? Maybe the function should set scheduleRelayout to false if isInPerformLayout() at the top? Thank you, that works good. I'll see if this sticks, and then send a clean up CL for TextAutosizer as those fixes are no longer necessary if this works good.
Description was changed from ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, we're marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908 ========== to ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, markContainerChainForLayout() is marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908 ==========
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1906803002/#ps40001 (title: "leviw/cbiesinger reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1906803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1906803002/40001
https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:758: // Also, schedduleRelayout() must not be called while iterating scheddule -> schedule
The CQ bit was unchecked by kojii@chromium.org
On 2016/04/26 at 02:04:30, cbiesinger wrote: > https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:758: // Also, schedduleRelayout() must not be called while iterating > scheddule -> schedule Thanks, thought silence meant go, sorry about that.
On 2016/04/26 02:05:49, kojii wrote: > On 2016/04/26 at 02:04:30, cbiesinger wrote: > > > https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > > > > https://codereview.chromium.org/1906803002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:758: // Also, > schedduleRelayout() must not be called while iterating > > scheddule -> schedule > > Thanks, thought silence meant go, sorry about that. Oh, you were right in assuming that, I just happened to notice this typo just now :)
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1906803002/#ps60001 (title: "Fix typo "scheddule"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1906803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1906803002/60001
Description was changed from ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, markContainerChainForLayout() is marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908 ========== to ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, markContainerChainForLayout() is marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908, 589773 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, markContainerChainForLayout() is marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908, 589773 ========== to ========== Prevent markContainerChainForLayout to scheduleRelayout() if isInPerformLayout() When we're in layout, markContainerChainForLayout() is marking a descendant as needing layout with the intention of visiting it during this layout. We shouldn't be scheduling it to be laid out later. Also, schedduleRelayout() must not be called while iterating FrameView::m_layoutSubtreeRootList. BUG=602908, 589773 Committed: https://crrev.com/1a0aa2dff16abbd9bc7f906b4d807766668bde17 Cr-Commit-Position: refs/heads/master@{#389691} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1a0aa2dff16abbd9bc7f906b4d807766668bde17 Cr-Commit-Position: refs/heads/master@{#389691} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
