| Summary: | Reduce PassRefPtr uses in dom - 2 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | ap, buildbot, cdumez, commit-queue, esprehn+autocc, kangil.han, rniwa, ryanhaddad, WebkitBugTracker | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 152391 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Gyuyoung Kim
2015-12-07 02:27:27 PST
Created attachment 266760 [details]
Patch
Created attachment 267290 [details]
Patch
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. Created attachment 267412 [details]
Patch
(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. 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. Committed r194201: <http://trac.webkit.org/changeset/194201> (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. 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 Re-opened since this is blocked by bug 152391 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? 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! Created attachment 267604 [details]
Patch
(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. 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. 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
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. 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
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. 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
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 Created attachment 267608 [details]
Patch
Comment on attachment 267608 [details] Patch Clearing flags on attachment: 267608 Committed r194324: <http://trac.webkit.org/changeset/194324> All reviewed patches have been landed. Closing bug. |