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.
Created attachment 163201 [details] Patch
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())
Adding Dimitri as he can answer your shadow root question
Created attachment 163357 [details] Patch
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 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.
Created attachment 163380 [details] Patch
Looks good to me.
Comment on attachment 163380 [details] Patch Clearing flags on attachment: 163380 Committed r128222: <http://trac.webkit.org/changeset/128222>
All reviewed patches have been landed. Closing bug.
Mass move bugs into the DOM component.