Changeset 217610 in webkit

Timestamp:

May 31, 2017 10:49:04 AM (7 years ago)

Author:

fred.wang@free.fr

Message:

ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits is wrongly set
​https://bugs.webkit.org/show_bug.cgi?id=172349

Patch by Frederic Wang <​fwang@igalia.com> on 2017-05-31
Reviewed by Simon Fraser.

ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits was introduced in
r133022 so that ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode
know the number of bits use for properties in their parent class.

In r172649, r210560, r185762 and r183702 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. It is not
clear how to write a test case reproducing such conflict, though.

No new tests, this is a coding mistake but its effect is unclear.

• page/scrolling/ScrollingStateNode.cpp:

(WebCore::ScrollingStateNode::setPropertyChanged): Use hasChangedProperty and cast to
64-bits integer before shifting.

• page/scrolling/ScrollingStateNode.h: Add a comment to make clear NumStateNodeBits must

remain at the last position. Ensure we have enough bits available.
(WebCore::ScrollingStateNode::hasChangedProperty): Cast to 64-bits integer before shifting.

• page/scrolling/ScrollingStateScrollingNode.h: Fix position of NumScrollingStateNodeBits and

also add a similar comment.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ScrollingStateNode.cpp (modified) (1 diff)
• page/scrolling/ScrollingStateNode.h (modified) (2 diffs)
• page/scrolling/ScrollingStateScrollingNode.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-05-31  Frederic Wang  <fwang@igalia.com>
+
+        ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits is wrongly set
+        https://bugs.webkit.org/show_bug.cgi?id=172349
+
+        Reviewed by Simon Fraser.
+
+        ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits was introduced in
+        r133022 so that ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode
+        know the number of bits use for properties in their parent class.
+
+        In r172649, r210560, r185762 and r183702 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. It is not
+        clear how to write a test case reproducing such conflict, though.
+
+        No new tests, this is a coding mistake but its effect is unclear.
+
+        * page/scrolling/ScrollingStateNode.cpp:
+        (WebCore::ScrollingStateNode::setPropertyChanged): Use hasChangedProperty and cast to
+        64-bits integer before shifting.
+        * page/scrolling/ScrollingStateNode.h: Add a comment to make clear NumStateNodeBits must
+        remain at the last position. Ensure we have enough bits available.
+        (WebCore::ScrollingStateNode::hasChangedProperty): Cast to 64-bits integer before shifting.
+        * page/scrolling/ScrollingStateScrollingNode.h: Fix position of NumScrollingStateNodeBits and
+        also add a similar comment.
+
 2017-05-31  Matt Lewis  <jlewis3@apple.com>
 

trunk/Source/WebCore/page/scrolling/ScrollingStateNode.cpp

@@ -66 +66 @@
 void ScrollingStateNode::setPropertyChanged(unsigned propertyBit)
 {
-    if (m_changedProperties & (1 << propertyBit))
+    if (hasChangedProperty(propertyBit))
         return;
 
-    m_changedProperties |= (1 << propertyBit);
+    m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit);
     m_scrollingStateTree.setHasChangedProperties();
 }

trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h

@@ -30 +30 @@
 #include "GraphicsLayer.h"
 #include "ScrollingCoordinator.h"
+#include <stdint.h>
 #include <wtf/RefCounted.h>
 #include <wtf/TypeCasts.h>
@@ -203 +204 @@
     enum {
         ScrollLayer = 0,
-        NumStateNodeBits = 1
+        NumStateNodeBits // This must remain at the last position.
     };
-    typedef unsigned ChangedProperties;
+    typedef uint64_t ChangedProperties;
 
     bool hasChangedProperties() const { return m_changedProperties; }
-    bool hasChangedProperty(unsigned propertyBit) const { return m_changedProperties & (1 << propertyBit); }
+    bool hasChangedProperty(unsigned propertyBit) const { return m_changedProperties & (static_cast<ChangedProperties>(1) << propertyBit); }
     void resetChangedProperties() { m_changedProperties = 0; }
     void setPropertyChanged(unsigned propertyBit);

trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h

@@ -47 +47 @@
         ScrollableAreaParams,
         RequestedScrollPosition,
-        NumScrollingStateNodeBits,
 #if ENABLE(CSS_SCROLL_SNAP)
         HorizontalSnapOffsets,
@@ -57 +56 @@
 #endif
         ExpectsWheelEventTestTrigger,
+        NumScrollingStateNodeBits // This must remain at the last position.
     };