Summary: | Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, kangil.han, simon.fraser, thorton, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2015-04-04 15:18:53 PDT
Created attachment 250139 [details]
Patch
Comment on attachment 250139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250139&action=review > Source/WebCore/dom/Document.cpp:6142 > - > + This seems like an unnecessary whitespace line hange. > Source/WebCore/dom/Node.cpp:331 > if (hasEventTargetData()) { > + document.didRemoveWheelEventHandler(*this); I don't think this is right. Look at Node::didMoveToNewDocument. We're checking the number of wheel event listeners and calling didRemoveWheelEventHandler as many times as needed. I bet you can create a test case with multiple wheel event listeners that still crash. In fact, m_wheelEventTargets is a HashCountedSet<Node*> Created attachment 250140 [details]
Patch
Comment on attachment 250140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250140&action=review > Source/WebCore/dom/Document.cpp:6006 > +// Returns true if the Node was removed from the set. I don't think we need this comment. This is a very common pattern in WebCore. If anything we can add a new enum class. > Source/WebCore/dom/Document.cpp:6015 > + Nit: whitespace. > Source/WebCore/dom/Document.cpp:6072 > -void Document::didRemoveTouchEventHandler(Node& handler) > +void Document::didRemoveTouchEventHandler(Node& handler, EventHandlerRemoval removal) Looks like we're never calling this function with EventHandlerRemoval::All? > Source/WebCore/dom/Document.cpp:6155 > - > + Nit: unrelated whitespace change. > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/destroy-element-with-multiple-handlers-crash-expected.txt:1 > +This test passes if it does not crash. Please add a description as to what we're testing. > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/destroy-element-with-multiple-handlers-crash.html:21 > + Nit: whitespace. It looks like all these tests have the same issue. > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/destroy-wheel-element-crash-expected.txt:1 > +This test passes if it does not crash. Ditto. |