Bug 168018 – [Cocoa] Web Inspector: Inspector::fromProtocolString<T> should return std::optional<T>

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

[Blaze Burg]

Currently we assert if the enum value cannot be parsed, but this is untrusted data. We should handle it better.

[Radar WebKit Bug Importer]

<rdar://problem/30468779>

[Blaze Burg]

Created attachment 301210 [details]
Proposed Fix

Must land manually with internal changes.

[Blaze Burg]

Created attachment 301223 [details]
Proposed Fix (rebased)

[Blaze Burg]

Created attachment 301635 [details]
Proposed Fix (rebased)

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Joseph Pecoraro]

Comment on attachment 301635 [details]
Proposed Fix (rebased)

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

> Source/JavaScriptCore/inspector/scripts/tests/generic/expected/commands-with-optional-call-return-parameters.json-result:927
> +    std::optional<TestProtocolDatabasePrimaryColors> o_in_screenColor = fromProtocolString<TestProtocolDatabasePrimaryColors>(in_screenColor);
> +    THROW_EXCEPTION_FOR_BAD_ENUM_VALUE(o_in_screenColor, @"screenColor");
>      NSArray/*<NSString>*/ *o_in_alternateColors = objcStringArray(&in_alternateColors);
> -    TestProtocolDatabaseExecuteNoOptionalParametersPrintColor o_in_printColor = fromProtocolString<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor>(in_printColor);
> +    std::optional<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor> o_in_printColor = fromProtocolString<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor>(in_printColor);
> +    THROW_EXCEPTION_FOR_BAD_ENUM_VALUE(o_in_printColor, @"printColor");

It does not make sense to throw an exception here.

This is the C++ code reading an incoming messages from the frontend to then dispatch an ObjC Backend Dispatcher. No client code is wrapping this, this is entirely WebKit receiving a frontend message. Nobody will catch the exception or handle it, it is our responsibility to do something here.

If this is bad input from the inspector, it should be rejected like any other bad protocol input. For example:

    if (!o_in_screenColor) {
        m_backendDispatcher->reportProtocolError(BackendDispatcher::InvalidParams, String::format("Some arguments of method '%s' can't be processed", "Database.executeNoOptionalParameters"));
        return;
    }

[Blaze Burg]

(In reply to comment #6)
> Comment on attachment 301635 [details]
> Proposed Fix (rebased)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301635&action=review
> 
> > Source/JavaScriptCore/inspector/scripts/tests/generic/expected/commands-with-optional-call-return-parameters.json-result:927
> > +    std::optional<TestProtocolDatabasePrimaryColors> o_in_screenColor = fromProtocolString<TestProtocolDatabasePrimaryColors>(in_screenColor);
> > +    THROW_EXCEPTION_FOR_BAD_ENUM_VALUE(o_in_screenColor, @"screenColor");
> >      NSArray/*<NSString>*/ *o_in_alternateColors = objcStringArray(&in_alternateColors);
> > -    TestProtocolDatabaseExecuteNoOptionalParametersPrintColor o_in_printColor = fromProtocolString<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor>(in_printColor);
> > +    std::optional<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor> o_in_printColor = fromProtocolString<TestProtocolDatabaseExecuteNoOptionalParametersPrintColor>(in_printColor);
> > +    THROW_EXCEPTION_FOR_BAD_ENUM_VALUE(o_in_printColor, @"printColor");
> 
> It does not make sense to throw an exception here.
> 
> This is the C++ code reading an incoming messages from the frontend to then
> dispatch an ObjC Backend Dispatcher. No client code is wrapping this, this
> is entirely WebKit receiving a frontend message. Nobody will catch the
> exception or handle it, it is our responsibility to do something here.
> 
> If this is bad input from the inspector, it should be rejected like any
> other bad protocol input. For example:
> 
>     if (!o_in_screenColor) {
>        
> m_backendDispatcher->reportProtocolError(BackendDispatcher::InvalidParams,
> String::format("Some arguments of method '%s' can't be processed",
> "Database.executeNoOptionalParameters"));
>         return;
>     }

Right, I did not fully understand what this code was doing, but I think I have a much better picture now. Since this fails when handling an incoming request, the failure path should reply to the command and not call into ObjC alternate dispatcher at all. I think it would be more consistent to call errorCallback() directly and return.

[Blaze Burg]

Created attachment 301777 [details]
Patch v2

[Blaze Burg]

To test:
 - Go to settings tabs
 - Toggle DebugUI in engineering build (Cmd-Shift-Opt + D)
 - Use the select to change layout direction value

Behavior:
 - Turning DebugUI on should make the setting appear immediately.
 - Turning DebugUI back off should make the setting disappear immediately.
 - Changing the value should cause the inspector frontend to reload immediately.
 - The reloaded UI should have the specified layout direction.

[Blaze Burg]

(In reply to comment #9)
> To test:
>  - Go to settings tabs
>  - Toggle DebugUI in engineering build (Cmd-Shift-Opt + D)
>  - Use the select to change layout direction value
> 
> Behavior:
>  - Turning DebugUI on should make the setting appear immediately.
>  - Turning DebugUI back off should make the setting disappear immediately.
>  - Changing the value should cause the inspector frontend to reload
> immediately.
>  - The reloaded UI should have the specified layout direction.

Wrong bug.

[Joseph Pecoraro]

Comment on attachment 301777 [details]
Patch v2

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

r=me

> Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py:202
> +                # FIXME: we don't handle optional enum values in commands here because it isn't used anywhere yet.
> +                # We'd need to change the delegate's signature to take std::optional for optional enum values.

I don't think we'd ever want the delegate to receive a std::optional. The delegate is an ObjC API and this would require it to be an ObjC++ and use of std::optional is unexpected for ObjC APIs.

[Blaze Burg]

Comment on attachment 301777 [details]
Patch v2

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

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py:202
>> +                # We'd need to change the delegate's signature to take std::optional for optional enum values.
> 
> I don't think we'd ever want the delegate to receive a std::optional. The delegate is an ObjC API and this would require it to be an ObjC++ and use of std::optional is unexpected for ObjC APIs.

Er, good catch. In that case we'd probably need to box it as a NSNumber and allow it to be nil. I'll just remove the second comment line.

[Blaze Burg]

WK1 failures do not look related, maybe fallout from import() patch?

[Joseph Pecoraro]

(In reply to comment #13)
> WK1 failures do not look related, maybe fallout from import() patch?

Correct, and Yusuke has landed some follow-ups for that. (Maybe not enough?)

[Blaze Burg]

Committed r212462: <http://trac.webkit.org/changeset/212462>