Bug 219378 – Web Inspector: InspectorFrontendAPIDispatcher should not ignore all exceptions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This makes it extremely difficult to debug problems when hacking, and it would be good to at least log exceptions that are raised by internal evaluations.

[Blaze Burg]

Created attachment 415074 [details]
Patch v1

[Blaze Burg]

I noticed a few tests timing out locally, so I'm interested to see if this happens on the bots. In theory it shouldn't have caused tests to be any slower because it's still calling the underlying ScriptController method.

[Devin Rousso]

Comment on attachment 415074 [details]
Patch v1

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

r=mews

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:328
> +Optional<bool> InspectorFrontendClientLocal::evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult result)

Rather than `Optional<bool>` should we always just `.valueOr(false)`?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:334
> +    if (valueOrException)

I'd make this `if (!valueOrException)` so that we early-return for failure (that seems to be our usual pattern too)

> Source/WebCore/inspector/InspectorFrontendClientLocal.h:137
> +    Optional<bool> evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult);

Why not make this `static` in the `.cpp`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:59
> +    if (!result.has_value()) {

I don't think the `has_value` is needed as `operator bool` does the same thing

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:65
> +        default:

NIT: I personally prefer avoiding `default` so that if new enum values are added then there's a compile error (which brings visibility to the usage here, causing the programmer to make a conscious decision about it instead of it being silent)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:77
> +    if (!valueOrException.has_value()) {

ditto (:59)

[Blaze Burg]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 415074 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415074&action=review
> 
> r=mews
> 
> > Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:328
> > +Optional<bool> InspectorFrontendClientLocal::evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult result)
> 
> Rather than `Optional<bool>` should we always just `.valueOr(false)`?

I was hesitant, but it's probably OK since this is a private method only used by this class.

> 
> > Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:334
> > +    if (valueOrException)
> 
> I'd make this `if (!valueOrException)` so that we early-return for failure
> (that seems to be our usual pattern too)
> 
> > Source/WebCore/inspector/InspectorFrontendClientLocal.h:137
> > +    Optional<bool> evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult);
> 
> Why not make this `static` in the `.cpp`?

To call JSC::JSValue::toBoolean(), we need to access the global object via m_frontendAPIDispatcher. So we'd have to pass around that context anyway. Easier to make it an instance method IMO.


> > Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:59
> > +    if (!result.has_value()) {
> 
> I don't think the `has_value` is needed as `operator bool` does the same
> thing

Given that this patch deals with JSValues that could be `true` or `false`, I found it hard to quickly understand when operator bool is checking the truthiness of JSValue or the Optional. This makes it explicit.

> 
> > Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:65
> > +        default:
> 
> NIT: I personally prefer avoiding `default` so that if new enum values are
> added then there's a compile error (which brings visibility to the usage
> here, causing the programmer to make a conscious decision about it instead
> of it being silent)

OK

> > Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:77
> > +    if (!valueOrException.has_value()) {
> 
> ditto (:59)

[Blaze Burg]

Created attachment 415429 [details]
Patch v1.1 - Fix failing test.

[Blaze Burg]

Created attachment 415437 [details]
Patch v1.2 - fix Debug Mac

[EWS]

Committed r270453: <https://trac.webkit.org/changeset/270453>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415437 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71990522>