Bug 172349

Summary:

ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits is wrongly set

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

WebCore Misc.

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, buildbot, cmarcelo, jamesr, luiz, rniwa, simon.fraser, tonikitoo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

133022, 135769, 135994, 144482, 145318

Bug Blocks:

171667

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
Patch	none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch	simon.fraser: review+

Frédéric Wang (:fredw)

Reported 2017-05-19 01:04:53 PDT

ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits was introduced in bug 133022 so that ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode know the number of bits use for properties in their parent class. In bug 135769, bug 135994, bug 145318 and bug 144482, new properties were added to ScrollingStateScrollingNode but NumScrollingStateNodeBits was not increased accordingly. This means that there are potential conflicts between these new properties and those of derived classes ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode. Not sure if we have some concrete use case demonstrating the issue, though.

Attachments
Patch (3.07 KB, patch) 2017-05-19 01:18 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.12 MB, application/zip) 2017-05-19 02:27 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (916.59 KB, application/zip) 2017-05-19 02:49 PDT, Build Bot	no flags	Details
Patch (3.50 KB, patch) 2017-05-19 07:38 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.42 MB, application/zip) 2017-05-19 08:41 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.35 MB, application/zip) 2017-05-19 09:30 PDT, Build Bot	no flags	Details
Patch (4.79 KB, patch) 2017-05-21 11:38 PDT, Frédéric Wang (:fredw)	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2017-05-19 01:18:10 PDT

Created attachment 310632 [details] Patch

Build Bot

Comment 2 2017-05-19 02:27:37 PDT

Comment on attachment 310632 [details] Patch Attachment 310632 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3775286 New failing tests: fast/css/sticky/sticky-margins.html fast/css/sticky/sticky-writing-mode-vertical-rl.html media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html tiled-drawing/scrolling/fixed-background/fixed-background-composited.html fast/css/sticky/sticky-top-margins.html fast/css/sticky/sticky-left.html fast/css/sticky/sticky-as-positioning-container.html fast/css/sticky/sticky-table-row-top.html fast/css/sticky/sticky-left-percentage.html tiled-drawing/scrolling/fixed-background/fixed-background-negative-z-index-fixed.html fast/scrolling/rtl-scrollbars-sticky-document-2.html fast/css/sticky/sticky-table-thead-top.html fast/css/sticky/sticky-top.html fast/css/sticky/sticky-side-margins.html fast/css/sticky/inflow-sticky.html fast/css/sticky/sticky-writing-mode-vertical-lr.html fast/css/sticky/sticky-writing-mode-horizontal-bt.html fast/repaint/fixed-scale.html fast/repaint/fixed.html fast/css/sticky/replaced-sticky.html

Build Bot

Comment 3 2017-05-19 02:27:39 PDT

Created attachment 310638 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 4 2017-05-19 02:49:28 PDT

Comment on attachment 310632 [details] Patch Attachment 310632 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3775307 New failing tests: scrollingcoordinator/ios/ui-scrolling-tree.html

Build Bot

Comment 5 2017-05-19 02:49:30 PDT

Created attachment 310640 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Frédéric Wang (:fredw)

Comment 6 2017-05-19 07:38:13 PDT

Created attachment 310655 [details] Patch

Build Bot

Comment 7 2017-05-19 08:41:14 PDT

Comment on attachment 310655 [details] Patch Attachment 310655 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3776868 New failing tests: fast/css/sticky/sticky-margins.html fast/css/sticky/sticky-writing-mode-vertical-lr.html media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html tiled-drawing/scrolling/fixed-background/fixed-background-composited.html fast/css/sticky/sticky-top-margins.html fast/css/sticky/sticky-left.html fast/css/sticky/sticky-as-positioning-container.html fast/css/sticky/sticky-table-row-top.html fast/css/sticky/sticky-left-percentage.html tiled-drawing/scrolling/fixed-background/fixed-background-negative-z-index-fixed.html fast/scrolling/rtl-scrollbars-sticky-document-2.html fast/css/sticky/sticky-table-thead-top.html fast/css/sticky/sticky-top.html fast/css/sticky/inflow-sticky.html fast/css/sticky/sticky-writing-mode-vertical-rl.html fast/css/sticky/sticky-writing-mode-horizontal-bt.html fast/repaint/fixed.html fast/repaint/fixed-scale.html fast/css/sticky/sticky-side-margins.html fast/css/sticky/replaced-sticky.html

Build Bot

Comment 8 2017-05-19 08:41:16 PDT

Created attachment 310660 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 9 2017-05-19 09:30:22 PDT

Comment on attachment 310655 [details] Patch Attachment 310655 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3776989 New failing tests: scrollingcoordinator/ios/ui-scrolling-tree.html

Build Bot

Comment 10 2017-05-19 09:30:24 PDT

Created attachment 310663 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Simon Fraser (smfr)

Comment 11 2017-05-19 10:10:33 PDT

Comment on attachment 310655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310655&action=review Nice find! > Source/WebCore/page/scrolling/ScrollingStateNode.h:208 > + typedef uint64_t ChangedProperties; There's no actual need to do this change now, right? We never have more than 32 bits.

Frédéric Wang (:fredw)

Comment 12 2017-05-19 10:13:46 PDT

(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 310655 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310655&action=review > > Nice find! > > > Source/WebCore/page/scrolling/ScrollingStateNode.h:208 > > + typedef uint64_t ChangedProperties; > > There's no actual need to do this change now, right? We never have more than > 32 bits. Mmh, I think we do (if we sum up the properties of ScrollingNode + FrameScrollingNode) and that change seemed to fix the failures in my first patch when I tested locally... Anyway, will go back to this next week.

Frédéric Wang (:fredw)

Comment 13 2017-05-21 11:38:03 PDT

Created attachment 310814 [details] Patch

Konstantin Tokarev

Comment 14 2017-05-23 09:07:38 PDT

Comment on attachment 310814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310814&action=review > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:71 > + m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit); It may be cleaner to use BitVector or FastBitVector for m_changedProperties, or, if it's not acceptable performance-wise, introduce non-resizable BitVector-like template that uses given type (e.g. uint64_t) for storage

Frédéric Wang (:fredw)

Comment 15 2017-05-30 22:51:00 PDT

(In reply to Konstantin Tokarev from comment #14) > It may be cleaner to use BitVector or FastBitVector for m_changedProperties, > or, if it's not acceptable performance-wise, introduce non-resizable > BitVector-like template that uses given type (e.g. uint64_t) for storage Thank you, indeed the current setup is a bit fragile, this patch was just intended to fix the obvious error without modifying too much the code. @Simon: What do you think? Is it worth following that suggestion now? Is there any performance concern?

Frédéric Wang (:fredw)

Comment 16 2017-05-31 00:45:24 PDT

Comment on attachment 310814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310814&action=review >> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:71 >> + m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit); > > It may be cleaner to use BitVector or FastBitVector for m_changedProperties, or, if it's not acceptable performance-wise, introduce non-resizable BitVector-like template that uses given type (e.g. uint64_t) for storage So BitVector/FastBitVector seems a bit overkill since we just use a bit more than 32bits for now and they don't have a fast helper to check ScrollingStateNode::hasChangedProperties.

Simon Fraser (smfr)

Comment 17 2017-05-31 10:36:27 PDT

Comment on attachment 310814 [details] Patch Let's land this now and move to BitVector or something in another patch.

Frédéric Wang (:fredw)

Comment 18 2017-05-31 10:49:13 PDT

Committed r217610: <http://trac.webkit.org/changeset/217610>

Radar WebKit Bug Importer

Comment 19 2017-05-31 10:50:18 PDT

<rdar://problem/32489809>

Note You need to log in before you can comment on or make changes to this bug.