Bug 96313 – Crash on a long press gesture when touch adjustment is enabled.

• Component: DOM
• Status: RESOLVED FIXED

[Kevin Ellis]

Steps to reproduce:
  1. Navigate to www.w3shcools.com/html5/tryit.asp?filename=tryhtml5_audio_all
  2. Long press (touch) on the mute button in the audio control.

Crashes when determining the best adjusted node for the context menu.  The mute button is a shadow DOM element and node->renderer() is null inside of TouchAdjustment::providesContextMenuItems.

[Kevin Ellis]

Created attachment 163201 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 163201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163201&action=review

> Source/WebCore/page/TouchAdjustment.cpp:104
> +    if (node->isShadowRoot())
> +        return false;

So the problem is that node->renderer() can be 0 if it is a shadowRoot?
Perhaps checking for !node->renderer() would be safer and more informative. Especially if combined with ASSERT(node->renderer() || node->isShadowRoot())

[Kenneth Rohde Christiansen]

Adding Dimitri as he can answer your shadow root question

[Kevin Ellis]

Created attachment 163357 [details]
Patch

[Kevin Ellis]

Comment on attachment 163201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163201&action=review

>> Source/WebCore/page/TouchAdjustment.cpp:104
>> +        return false;
> 
> So the problem is that node->renderer() can be 0 if it is a shadowRoot?
> Perhaps checking for !node->renderer() would be safer and more informative. Especially if combined with ASSERT(node->renderer() || node->isShadowRoot())

Yes node->renderer() can be null now that we enable traversal into shadow-DOM elements during hit testing.  Without the check, the test added for this CL crashes.

[Allan Sandfeld Jensen]

Comment on attachment 163357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163357&action=review

This could still crash later in appendBasicSubtargetsForNode.

> Source/WebCore/page/TouchAdjustment.cpp:105
> +    ASSERT(node->renderer() || node->isShadowRoot());
> +    if (!node->renderer())
> +        return false;

The check will need to be in the top of the function. If node does not have a rendered, the node must always be filtered. 

The existence of the renderer is asserted in appendBasicSubtargetsForNode, and you need to update the comment for that assertion as well, telling that it is guaranteed due to the check in the node filter.

[Kevin Ellis]

Created attachment 163380 [details]
Patch

[Allan Sandfeld Jensen]

Looks good to me.

[WebKit Review Bot]

Comment on attachment 163380 [details]
Patch

Clearing flags on attachment: 163380

Committed r128222: <http://trac.webkit.org/changeset/128222>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass move bugs into the DOM component.