[webkit-reviews] review denied: [Bug 108636] Creating a WebInspector.ContextMenu without an event crashes WebCore when calling .show() : [Attachment 186066] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 108636: Creating a WebInspector.ContextMenu without an event crashes
WebCore when calling .show()
https://bugs.webkit.org/show_bug.cgi?id=108636

Attachment 186066: Patch
https://bugs.webkit.org/attachment.cgi?id=186066&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186066&action=review


>>>> Source/WebCore/page/ContextMenuController.cpp:111
>>>> +	  if (!event)
>>> 
>>> When does this happen?
>> 
>> I'm not sure when it would, but I understood from your previous comment you
wanted all call sites to ContextMenuController::createContextMenu() guarded.
> 
> I guess you meant the actual original call site in the JS code. In this case,
WebInspector.ContextMenu.prototype.show().

Oh, I meant to place the checks into the call sites that operate nullable event
pointers. This one does not.

>>> Source/WebCore/page/ContextMenuController.cpp:130
>>> +	 if (!event)
>> 
>> When does this happen?
> 
> This would happen if you call InspectorFrontendHost.showContextMenu() from
the front-end JS code without an event. This was how I originally found the
issue.

You probably want to move this check into the call site
(InspectorFrontendHost::showContextMenu) then. That way you clearly separate
contexts with nullable event pointers from the ones that rely upon their
existence.