Bug 172349 – ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits is wrongly set

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 310632 [details]
Patch

[Build Bot]

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]

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 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]

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)]

Created attachment 310655 [details]
Patch

[Build Bot]

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]

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 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]

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 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)]

(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)]

Created attachment 310814 [details]
Patch

[Konstantin Tokarev]

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)]

(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 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 on attachment 310814 [details]
Patch

Let's land this now and move to BitVector or something in another patch.

[Frédéric Wang (:fredw)]

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

[Radar WebKit Bug Importer]

<rdar://problem/32489809>