Bug 143406

Summary: Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch rniwa: review+

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]
Patch
Comment 2 Simon Fraser (smfr) 2015-04-04 15:23:21 PDT
rdar://problem/20407080
Comment 3 Ryosuke Niwa 2015-04-04 16:25:21 PDT
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*>
Comment 4 Simon Fraser (smfr) 2015-04-04 17:39:10 PDT
Created attachment 250140 [details]
Patch
Comment 5 Ryosuke Niwa 2015-04-04 17:45:52 PDT
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.
Comment 6 Simon Fraser (smfr) 2015-04-04 17:54:29 PDT
https://trac.webkit.org/r182349
https://trac.webkit.org/r182350