Bug 151936 – Reduce PassRefPtr uses in dom - 2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 266760 [details]
Patch

[Gyuyoung Kim]

Created attachment 267290 [details]
Patch

[Darin Adler]

Comment on attachment 267290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267290&action=review

review- since most of these are wrong/overusing RefPtr

> Source/WebCore/dom/ScriptedAnimationController.h:64
> +    CallbackId registerCallback(RefPtr<RequestAnimationFrameCallback>&&);

Should be Ref<> not RefPtr<> since it can’t be null.

> Source/WebCore/dom/Traversal.h:44
> +        NodeIteratorBase(RefPtr<Node>&&, unsigned long whatToShow, RefPtr<NodeFilter>&&);

Should just be a Node* or a Node& if it’s never null, since nobody ever hands us ownership when making one of these objects.

> Source/WebCore/dom/TreeWalker.h:40
> +        static Ref<TreeWalker> create(RefPtr<Node>&& rootNode, unsigned long whatToShow, RefPtr<NodeFilter>&& filter)

Should just be a Node* or a Node& if it’s never null, since nobody ever hands us ownership when making one of these objects.

> Source/WebCore/dom/TreeWalker.h:46
> +        void setCurrentNode(RefPtr<Node>&&, ExceptionCode&);

Should just be a Node* since the bindings do not hand us ownership when making one of these objects.

> Source/WebCore/dom/WebKitNamedFlow.h:51
> +    static Ref<WebKitNamedFlow> create(RefPtr<NamedFlowCollection>&& manager, const AtomicString& flowThreadName);

Should be NamedFlowCollection& since we never call this with null and we never hand over ownership.

> Source/WebCore/dom/WebKitNamedFlow.h:84
> +    WebKitNamedFlow(RefPtr<NamedFlowCollection>&&, const AtomicString&);

Should be NamedFlowCollection& since we never call this with null and we never hand over ownership.

> Source/WebCore/dom/WheelEvent.h:67
> +    static Ref<WheelEvent> create(const PlatformWheelEvent& event, RefPtr<AbstractView>&& view)

Should be AbstractView* since we never hand over ownership.

> Source/WebCore/dom/WheelEvent.h:72
> +    void initWheelEvent(int rawDeltaX, int rawDeltaY, RefPtr<AbstractView>&&,

Should be AbstractView* since we never hand over ownership.

> Source/WebCore/dom/WheelEvent.h:76
> +    void initWebKitWheelEvent(int rawDeltaX, int rawDeltaY, RefPtr<AbstractView>&&,

Should be AbstractView* since we never hand over ownership.

> Source/WebCore/dom/WheelEvent.h:103
> +    WheelEvent(const PlatformWheelEvent&, RefPtr<AbstractView>&&);

Should be AbstractView* since we never hand over ownership.

[Gyuyoung Kim]

Created attachment 267412 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #3)
> Comment on attachment 267290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267290&action=review
> 
> review- since most of these are wrong/overusing RefPtr
> 
> > Source/WebCore/dom/ScriptedAnimationController.h:64
> > +    CallbackId registerCallback(RefPtr<RequestAnimationFrameCallback>&&);
> 
> Should be Ref<> not RefPtr<> since it can’t be null.

To do that, I need to modify CodeGeneratorJS.pm as well. It looks big work now. So I will do it in new bug next time.


> > Source/WebCore/dom/Traversal.h:44
> > +        NodeIteratorBase(RefPtr<Node>&&, unsigned long whatToShow, RefPtr<NodeFilter>&&);
> 
> Should just be a Node* or a Node& if it’s never null, since nobody ever
> hands us ownership when making one of these objects.
> 
> > Source/WebCore/dom/TreeWalker.h:40
> > +        static Ref<TreeWalker> create(RefPtr<Node>&& rootNode, unsigned long whatToShow, RefPtr<NodeFilter>&& filter)
> 
> Should just be a Node* or a Node& if it’s never null, since nobody ever
> hands us ownership when making one of these objects.
> 
> > Source/WebCore/dom/TreeWalker.h:46
> > +        void setCurrentNode(RefPtr<Node>&&, ExceptionCode&);
> 
> Should just be a Node* since the bindings do not hand us ownership when
> making one of these objects.
> 
> > Source/WebCore/dom/WebKitNamedFlow.h:51
> > +    static Ref<WebKitNamedFlow> create(RefPtr<NamedFlowCollection>&& manager, const AtomicString& flowThreadName);
> 
> Should be NamedFlowCollection& since we never call this with null and we
> never hand over ownership.
> 
> > Source/WebCore/dom/WebKitNamedFlow.h:84
> > +    WebKitNamedFlow(RefPtr<NamedFlowCollection>&&, const AtomicString&);
> 
> Should be NamedFlowCollection& since we never call this with null and we
> never hand over ownership.
> 
> > Source/WebCore/dom/WheelEvent.h:67
> > +    static Ref<WheelEvent> create(const PlatformWheelEvent& event, RefPtr<AbstractView>&& view)
> 
> Should be AbstractView* since we never hand over ownership.
> 
> > Source/WebCore/dom/WheelEvent.h:72
> > +    void initWheelEvent(int rawDeltaX, int rawDeltaY, RefPtr<AbstractView>&&,
> 
> Should be AbstractView* since we never hand over ownership.
> 
> > Source/WebCore/dom/WheelEvent.h:76
> > +    void initWebKitWheelEvent(int rawDeltaX, int rawDeltaY, RefPtr<AbstractView>&&,
> 
> Should be AbstractView* since we never hand over ownership.
> 
> > Source/WebCore/dom/WheelEvent.h:103
> > +    WheelEvent(const PlatformWheelEvent&, RefPtr<AbstractView>&&);
> 
> Should be AbstractView* since we never hand over ownership.

Fixed. Thanks.

[Darin Adler]

Comment on attachment 267412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267412&action=review

> Source/WebCore/dom/StaticNodeList.h:69
> +        return WTF::move(nodeList);

I don’t think we need WTF::move when it’s a return value like this. The “return value optimization” takes care of it.

[Gyuyoung Kim]

Committed r194201: <http://trac.webkit.org/changeset/194201>

[Gyuyoung Kim]

(In reply to comment #6)
> Comment on attachment 267412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267412&action=review
> 
> > Source/WebCore/dom/StaticNodeList.h:69
> > +        return WTF::move(nodeList);
> 
> I don’t think we need WTF::move when it’s a return value like this. The
> “return value optimization” takes care of it.

Ok, patch is landed after fixing it.

[Alexey Proskuryakov]

This caused crashes on GuardMalloc bots, will roll out.

CRASHING TEST: fast/repaint/4776765.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010f49ff54 WebCore::NamedFlowCollection::discardNamedFlow(WebCore::WebKitNamedFlow*) + 20
1   com.apple.WebCore             	0x000000010f90d9cf WebCore::WebKitNamedFlow::~WebKitNamedFlow() + 31
2   com.apple.WebCore             	0x000000010f0d7fac WTF::Ref<WebCore::DOMNamedFlowCollection>::~Ref() + 124
3   com.apple.JavaScriptCore      	0x000000010dda50e8 JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(JSC::MarkedBlock::BlockState)3, (JSC::MarkedBlock::SweepMode)1, true>() + 184

imported/w3c/web-platform-tests/dom/traversal/TreeWalker-currentNode.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010af2c82d WebCore::NodeIterator::~NodeIterator() + 13
1   com.apple.WebCore             	0x000000010acd515c WebCore::JSNodeIterator::destroy(JSC::JSCell*) + 44
2   com.apple.JavaScriptCore      	0x00000001098170e8 JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(JSC::MarkedBlock::BlockState)3, (JSC::MarkedBlock::SweepMode)1, true>() + 184
3   com.apple.JavaScriptCore      	0x0000000109816d7d JSC::MarkedBlock::FreeList JSC::MarkedBlock::sweepHelper<true>(JSC::MarkedBlock::SweepMode) + 173
4   com.apple.JavaScriptCore      	0x000000010916185b JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode) + 75
5   com.apple.JavaScriptCore      	0x0000000109161171 JSC::MarkedAllocator::allocateSlowCase(unsigned long) + 225
6   com.apple.JavaScriptCore      	0x0000000109161e90 JSC::JSString::create(JSC::VM&, WTF::PassRefPtr<WTF::StringImpl>) + 304

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152391

[Chris Dumez]

Comment on attachment 267412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267412&action=review

> Source/WebCore/dom/Traversal.h:48
> +        Node& m_root;

We are no longer ref'ing the root Node, why is this OK?

> Source/WebCore/dom/WebKitNamedFlow.h:93
> +    NamedFlowCollection& m_flowManager;

We are no longer ref'ing the flowManager, why is this OK?

[Darin Adler]

Comment on attachment 267412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267412&action=review

>> Source/WebCore/dom/Traversal.h:48
>> +        Node& m_root;
> 
> We are no longer ref'ing the root Node, why is this OK?

This change is wrong! How did I miss it!?

This need to be Ref<Node> or RefPtr<Node>, not Node&. The argument type being Node& is still correct, but the data member needs to be RefPtr or Ref. So sorry for missing this!

[Gyuyoung Kim]

Created attachment 267604 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #12)
> Comment on attachment 267412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267412&action=review
> 
> >> Source/WebCore/dom/Traversal.h:48
> >> +        Node& m_root;
> > 
> > We are no longer ref'ing the root Node, why is this OK?
> 
> This change is wrong! How did I miss it!?
> 
> This need to be Ref<Node> or RefPtr<Node>, not Node&. The argument type
> being Node& is still correct, but the data member needs to be RefPtr or Ref.
> So sorry for missing this!

I'm sorry for this too. The wrong uses are fixed.

[Build Bot]

Comment on attachment 267604 [details]
Patch

Attachment 267604 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/573878

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267605 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267604 [details]
Patch

Attachment 267604 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/573887

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267606 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267604 [details]
Patch

Attachment 267604 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/573893

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267607 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Chris Dumez]

Comment on attachment 267604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267604&action=review

> Source/WebCore/dom/Traversal.cpp:34
> +    : m_root(adoptRef(rootNode))

I don't think we want to adopt here, we want to ref the root node

> Source/WebCore/dom/WebKitNamedFlow.cpp:44
> +    , m_flowManager(adoptRef(manager))

Ditto

[Gyuyoung Kim]

Created attachment 267608 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267608 [details]
Patch

Clearing flags on attachment: 267608

Committed r194324: <http://trac.webkit.org/changeset/194324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.