Bug 143406 – Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard

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

[Simon Fraser (smfr)]

Crash under Document::absoluteRegionForEventTargets on build.webkit.org/dashboard

[Simon Fraser (smfr)]

Created attachment 250139 [details]
Patch

[Simon Fraser (smfr)]

rdar://problem/20407080

[Ryosuke Niwa]

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*>

[Simon Fraser (smfr)]

Created attachment 250140 [details]
Patch

[Ryosuke Niwa]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/r182349
https://trac.webkit.org/r182350