Bug 96313 - Crash on a long press gesture when touch adjustment is enabled.
Summary: Crash on a long press gesture when touch adjustment is enabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kevin Ellis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-10 13:46 PDT by Kevin Ellis
Modified: 2019-02-06 09:18 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.07 KB, patch)
2012-09-10 13:55 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (5.12 KB, patch)
2012-09-11 07:16 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (5.64 KB, patch)
2012-09-11 09:05 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ellis 2012-09-10 13:46:38 PDT
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.
Comment 1 Kevin Ellis 2012-09-10 13:55:29 PDT
Created attachment 163201 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-09-10 14:31:35 PDT
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())
Comment 3 Kenneth Rohde Christiansen 2012-09-10 15:12:46 PDT
Adding Dimitri as he can answer your shadow root question
Comment 4 Kevin Ellis 2012-09-11 07:16:39 PDT
Created attachment 163357 [details]
Patch
Comment 5 Kevin Ellis 2012-09-11 07:21:01 PDT
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.
Comment 6 Allan Sandfeld Jensen 2012-09-11 07:42:44 PDT
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.
Comment 7 Kevin Ellis 2012-09-11 09:05:38 PDT
Created attachment 163380 [details]
Patch
Comment 8 Allan Sandfeld Jensen 2012-09-11 09:18:09 PDT
Looks good to me.
Comment 9 WebKit Review Bot 2012-09-11 13:35:55 PDT
Comment on attachment 163380 [details]
Patch

Clearing flags on attachment: 163380

Committed r128222: <http://trac.webkit.org/changeset/128222>
Comment 10 WebKit Review Bot 2012-09-11 13:35:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Lucas Forschler 2019-02-06 09:18:32 PST
Mass move bugs into the DOM component.