Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Correctly handle scroll-snap-type changes to 'none'" #16724

Merged
merged 1 commit into from
May 8, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

This reverts commit 712c3cf3ed8201420acf23f760eaa34be20781cd.

Reason for revert: This patch causes webkit-layout-tests failure on WebKit_Linux_Trusty_ASAN bot:
https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Trusty%20ASAN/25720

Unexpected Failures:

  • external/wpt/css/css-scroll-snap/scroll-snap-type.html
  • virtual/threaded/external/wpt/css/css-scroll-snap/scroll-snap-type.html

STDERR: ==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200023f8d8 at pc 0x5620c924e56d bp 0x7ffde3c56830 sp 0x7ffde3c56828
STDERR: READ of size 8 at 0x61200023f8d8 thread T0 (content_shell)
STDERR: #0 0x5620c924e56c in get ./../../base/memory/scoped_refptr.h:212:27
STDERR: #1 0x5620c924e56c in Style ./../../third_party/blink/renderer/core/layout/layout_object.h:1615:0
STDERR: #2 0x5620c924e56c in GetPhysicalSnapType ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:88:0
STDERR: #3 0x5620c924e56c in blink::SnapCoordinator::UpdateSnapContainerData(blink::LayoutBox&) ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:107:0
STDERR: #4 0x5620c924e74b in blink::SnapCoordinator::UpdateAllSnapContainerData() ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:76:5

Original change's description:

Correctly handle scroll-snap-type changes to 'none'

Previously when a scroll container's snap type is changed to 'none' its
data was discarded including all of its snap areas. However this is
incorrect. Because while the snap type is 'none', the element is still
a scroll container which per spec [1] means that is should continue to
captures the snap areas in its subtree for whom it is the nearest
ancestor scroll container . The only difference is that it no longer
snaps.

The fix is that we no longer remove the snap container data just
because is has a 'none' snap type and instead keep it and its snap
areas. But we check the snap type before performing any snap.

To ensure this does not introduce any performance regression, this CL
also includes an optimization where we avoid re-calculating
snap_container_data when the snap type is 'none'. So keeping these snap
data should not be cheap.

Note that there is another problem where if the current snap container
is no longer a scroll container (e.g., overflow: scroll => overflow:
visible) we release its snap areas and they become "orphan". But if we
are to do this correctly, we should re-assign these areas to the next
stroller in the chain. Similarly when an element becomes a scroll
container, it can potentially take over snap areas from its parent snap
container.

This patch does not address that situation yet but fixes the easier
problem.

[1] https://drafts.csswg.org/css-scroll-snap/#overview

Bug: 953575
Test:

  • wpt/css/css-scroll-snap/scroll-snap-type-change.html => Changing snap-type should work correctly
  • wpt/css/css-scroll-snap/scroll-snap-type.html => Add a specific test for type 'none' to ensure it does not snap

Change-Id: Ie493ad68ecba818ed41c0ee103ccf44725ff6e3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589899
Reviewed-by: Majid Valipour <[email protected]>
Reviewed-by: David Bokan <[email protected]>
Commit-Queue: Majid Valipour <[email protected]>
Cr-Commit-Position: refs/heads/master@{#657460}

TBR=[email protected],[email protected]

Change-Id: I3a327f6e342e95d045194d24ceaf49de52b2b921
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 953575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600437
Reviewed-by: Takashi Sakamoto <[email protected]>
Commit-Queue: Takashi Sakamoto <[email protected]>
Cr-Commit-Position: refs/heads/master@{#657571}

This reverts commit 712c3cf3ed8201420acf23f760eaa34be20781cd.

Reason for revert: This patch causes webkit-layout-tests failure on WebKit_Linux_Trusty_ASAN bot:
https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Trusty%20ASAN/25720

Unexpected Failures:
* external/wpt/css/css-scroll-snap/scroll-snap-type.html
* virtual/threaded/external/wpt/css/css-scroll-snap/scroll-snap-type.html

STDERR: ==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200023f8d8 at pc 0x5620c924e56d bp 0x7ffde3c56830 sp 0x7ffde3c56828
STDERR: READ of size 8 at 0x61200023f8d8 thread T0 (content_shell)
STDERR:     #0 0x5620c924e56c in get ./../../base/memory/scoped_refptr.h:212:27
STDERR:     #1 0x5620c924e56c in Style ./../../third_party/blink/renderer/core/layout/layout_object.h:1615:0
STDERR:     #2 0x5620c924e56c in GetPhysicalSnapType ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:88:0
STDERR:     #3 0x5620c924e56c in blink::SnapCoordinator::UpdateSnapContainerData(blink::LayoutBox&) ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:107:0
STDERR:     #4 0x5620c924e74b in blink::SnapCoordinator::UpdateAllSnapContainerData() ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:76:5

Original change's description:
> Correctly handle scroll-snap-type changes to 'none'
>
>
> Previously when a scroll container's snap type is changed to 'none' its
> data was discarded including all of its snap areas. However this is
> incorrect. Because while the snap type is 'none', the element is still
> a scroll container which per spec [1] means  that is should continue to
> captures the snap areas in its subtree for whom it is the nearest
> ancestor scroll container . The only difference is that it no longer
> snaps.
>
> The fix is that we no longer remove the snap container data just
> because is has a 'none' snap type and instead keep it and its snap
> areas. But we check the snap type before performing any snap.
>
> To ensure this does not introduce any performance regression, this CL
> also includes an optimization where we avoid re-calculating
> snap_container_data when the snap type is 'none'. So keeping these snap
> data should not be cheap.
>
> Note that there is another problem where if the current snap container
> is no longer a scroll container (e.g., overflow: scroll => overflow:
> visible) we release its snap areas and they become "orphan". But if we
> are to do this correctly, we should re-assign these areas to the next
> stroller in the chain. Similarly when an element becomes a scroll
> container, it can potentially take over snap areas from its parent snap
> container.
>
>
> This patch does not address that situation yet but fixes the easier
> problem.
>
> [1] https://drafts.csswg.org/css-scroll-snap/#overview
>
> Bug: 953575
> Test:
>  - wpt/css/css-scroll-snap/scroll-snap-type-change.html => Changing snap-type should work correctly
>  - wpt/css/css-scroll-snap/scroll-snap-type.html => Add a specific test for type 'none' to ensure it does not snap
>
> Change-Id: Ie493ad68ecba818ed41c0ee103ccf44725ff6e3f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589899
> Reviewed-by: Majid Valipour <[email protected]>
> Reviewed-by: David Bokan <[email protected]>
> Commit-Queue: Majid Valipour <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#657460}

[email protected],[email protected]

Change-Id: I3a327f6e342e95d045194d24ceaf49de52b2b921
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 953575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600437
Reviewed-by: Takashi Sakamoto <[email protected]>
Commit-Queue: Takashi Sakamoto <[email protected]>
Cr-Commit-Position: refs/heads/master@{#657571}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 59ffeb6 into master May 8, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-31a914fc4b branch May 8, 2019 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants