Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720823002:
Fix crash in FrameView::updateLayersAndCompositingAfterScrollIfNeeded (Closed)
DescriptionFix crash in FrameView::updateLayersAndCompositingAfterScrollIfNeeded
The code was incorrectly assuming that the ancestor overflow layer would
always be non-null. However this method can be called during layout
before compositing inputs update, at which point the ancestor overflow
layer may not yet be set.
The correct approach is to just skip the sticky update in that case.
This still produces the correct behavior as the method will be called
again later during compositing inputs update.
BUG=696173
Review-Url: https://codereview.chromium.org/2720823002
Cr-Commit-Position: refs/heads/master@{#453612}
Committed: https://chromium.googlesource.com/chromium/src/+/33278697a0fa078a892468e69a90c6563d3ec137
Patch Set 1 #Patch Set 2 : Slight comment change #
Total comments: 4
Patch Set 3 : Run unittest under RLS as well #Patch Set 4 : Revert "Run unittest under RLS as well" #Patch Set 5 : Re-enabled RLS in test, use layoutViewportScrollableArea #
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run
smcgruer@chromium.org changed reviewers: + flackr@chromium.org, schenney@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ cannot get SignCLA result. Please try later.
https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:150: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) Don't we run into the same problem with updateLayerPositionsAfterOverflowScroll on non-root layers?
lgtm https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:150: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) On 2017/02/27 16:42:09, flackr wrote: > Don't we run into the same problem with updateLayerPositionsAfterOverflowScroll > on non-root layers? Nevermind, on that we update top-down so you didn't have the additional ancestor overflow checks.
The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@chromium.org changed reviewers: + bokan@chromium.org
lgtm % root-layer-scrolling bit in testing https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:150: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) I'm not sure I understand why we can't run this with RLS enabled. Sticky will still have to work in that world (though I'd believe it doesn't yet) so we should test there too. If RLS doesn't support sticky yet and so this test *can't* work, note that in the comment so that we know to remove the skip once RLS is far along enough.
https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2720823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:150: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) On 2017/02/28 13:07:44, bokan wrote: > I'm not sure I understand why we can't run this with RLS enabled. Sticky will > still have to work in that world (though I'd believe it doesn't yet) so we > should test there too. If RLS doesn't support sticky yet and so this test > *can't* work, note that in the comment so that we know to remove the skip once > RLS is far along enough. I guess its black-box testing. FrameView::updateLayersAndCompositingAfterScrollIfNeeded early-exits (immediately) if there are no viewport constrained objects, and as far as I understand it (which may be wrong), RLS implies no viewport constrained objects. However its still fair to say that the call shouldn't crash, so I have removed the special-case.
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2720823002/#ps40001 (title: "Run unittest under RLS as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Haha, apparently I should have defended my position more (and tried running the test under debug build). From FrameView::updateScrollOffset() (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...) if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) { // Don't scroll the FrameView! ASSERT_NOT_REACHED(); } So I was correct not to run this test under RLS. Will revert the latest PS and resubmit, unless anyone objects immediately.
On 2017/02/28 14:23:58, smcgruer wrote: > Haha, apparently I should have defended my position more (and tried running the > test under debug build). From FrameView::updateScrollOffset() > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...) > > if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) { > // Don't scroll the FrameView! > ASSERT_NOT_REACHED(); > } > > So I was correct not to run this test under RLS. Will revert the latest PS and > resubmit, unless anyone objects immediately. That's a tangentially related issue. You're scrolling using |document().view()->setScrollOffset|. This will fail in RLS because the root PLSA handles scrolling in that case. You can make this work in both RLS and non-RLS by using: |document().view()->layoutViewportScrollableArea()->setScrollOffset|
On 2017/02/28 14:29:14, bokan wrote: > On 2017/02/28 14:23:58, smcgruer wrote: > > Haha, apparently I should have defended my position more (and tried running > the > > test under debug build). From FrameView::updateScrollOffset() > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...) > > > > if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) { > > // Don't scroll the FrameView! > > ASSERT_NOT_REACHED(); > > } > > > > So I was correct not to run this test under RLS. Will revert the latest PS and > > resubmit, unless anyone objects immediately. > > That's a tangentially related issue. You're scrolling using > |document().view()->setScrollOffset|. This will fail in RLS because the root > PLSA handles scrolling in that case. You can make this work in both RLS and > non-RLS by using: > |document().view()->layoutViewportScrollableArea()->setScrollOffset| Ah, ok thanks. Done.
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2720823002/#ps80001 (title: "Re-enabled RLS in test, use layoutViewportScrollableArea")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488292785072680,
"parent_rev": "3a19cc97cac2fc1ed293257d330ded633773bb38", "commit_rev":
"33278697a0fa078a892468e69a90c6563d3ec137"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash in FrameView::updateLayersAndCompositingAfterScrollIfNeeded The code was incorrectly assuming that the ancestor overflow layer would always be non-null. However this method can be called during layout before compositing inputs update, at which point the ancestor overflow layer may not yet be set. The correct approach is to just skip the sticky update in that case. This still produces the correct behavior as the method will be called again later during compositing inputs update. BUG=696173 ========== to ========== Fix crash in FrameView::updateLayersAndCompositingAfterScrollIfNeeded The code was incorrectly assuming that the ancestor overflow layer would always be non-null. However this method can be called during layout before compositing inputs update, at which point the ancestor overflow layer may not yet be set. The correct approach is to just skip the sticky update in that case. This still produces the correct behavior as the method will be called again later during compositing inputs update. BUG=696173 Review-Url: https://codereview.chromium.org/2720823002 Cr-Commit-Position: refs/heads/master@{#453612} Committed: https://chromium.googlesource.com/chromium/src/+/33278697a0fa078a892468e69a90... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/33278697a0fa078a892468e69a90... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
