[webkit-reviews] review denied: [Bug 60616] Web Inspector: chromium renderer crash at eval in console : [Attachment 93283] [patch] second version with layout ttest

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 60616: Web Inspector: chromium renderer crash at eval in console
https://bugs.webkit.org/show_bug.cgi?id=60616

Attachment 93283: [patch] second version with layout ttest
https://bugs.webkit.org/attachment.cgi?id=93283&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93283&action=review

> LayoutTests/ChangeLog:12
> +	   * inspector/console/console-eval-crash-wk60616.html: Added.

In LayoutTests we put link to the bug into the test description. Test file name
doesn't include the bug number regularly.

> LayoutTests/inspector/console/console-eval-crash-wk60616.html:26
> +Test that any long api call from the frontend will not crash the inspected
page's renderer if the page is reloaded or frontend is closed in the middle.

Please add a link to the bug report.

> Source/WebCore/inspector/InspectorController.cpp:217
> +    m_inspectorBackendDispatcher = adoptRef(new InspectorBackendDispatcher(

Now that m_inspectorBackendDispatcher is a RefPtr we need to notify it that the
front-end has disconnected and that it shouldn't send any replies. We should
also check if it's possible that the InspectorController is destroyed during
message dispatching.

> Source/WebCore/inspector/InspectorController.cpp:393
> +    RefPtr<InspectorBackendDispatcher> dispatcher =
m_inspectorBackendDispatcher;

dispatcher -> protect like in other places in WebCore where we would like to
protect 'this' from being destroyed.