Bug 143406 - Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard
Summary: Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashb...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar
Depends on:
Reported: 2015-04-04 15:18 PDT by Simon Fraser (smfr)
Modified: 2015-04-04 17:54 PDT (History)
6 users (show)

See Also:

Patch (10.01 KB, patch)
2015-04-04 15:22 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (16.28 KB, patch)
2015-04-04 17:39 PDT, Simon Fraser (smfr)
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-04-04 15:18:53 PDT
Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard
Comment 1 Simon Fraser (smfr) 2015-04-04 15:22:22 PDT
Created attachment 250139 [details]
Comment 2 Simon Fraser (smfr) 2015-04-04 15:23:21 PDT
Comment 3 Ryosuke Niwa 2015-04-04 16:25:21 PDT
Comment on attachment 250139 [details]

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*>
Comment 4 Simon Fraser (smfr) 2015-04-04 17:39:10 PDT
Created attachment 250140 [details]
Comment 5 Ryosuke Niwa 2015-04-04 17:45:52 PDT
Comment on attachment 250140 [details]

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.

Comment 6 Simon Fraser (smfr) 2015-04-04 17:54:29 PDT