Bug 168018 - [Cocoa] Web Inspector: Inspector::fromProtocolString<T> should return std::optional<T>
Summary: [Cocoa] Web Inspector: Inspector::fromProtocolString<T> should return std::op...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-08 13:57 PST by Brian Burg
Modified: 2017-02-16 13:33 PST (History)
9 users (show)

See Also:


Attachments
Proposed Fix (65.29 KB, patch)
2017-02-10 16:12 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (rebased) (53.81 KB, patch)
2017-02-10 17:37 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (rebased) (65.75 KB, patch)
2017-02-15 11:22 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v2 (66.44 KB, patch)
2017-02-16 10:09 PST, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-02-08 13:57:36 PST
Currently we assert if the enum value cannot be parsed, but this is untrusted data. We should handle it better.
Comment 1 Radar WebKit Bug Importer 2017-02-10 11:29:11 PST
<rdar://problem/30468779>
Comment 2 Brian Burg 2017-02-10 16:12:25 PST
Created attachment 301210 [details]
Proposed Fix

Must land manually with internal changes.
Comment 3 Brian Burg 2017-02-10 17:37:30 PST
Created attachment 301223 [details]
Proposed Fix (rebased)
Comment 4 Brian Burg 2017-02-15 11:22:51 PST
Created attachment 301635 [details]
Proposed Fix (rebased)
Comment 5 WebKit Commit Bot 2017-02-15 11:24:02 PST
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`)
Comment 6 Joseph Pecoraro 2017-02-15 15:46:13 PST
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;
    }
Comment 7 Brian Burg 2017-02-16 09:15:53 PST
(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.
Comment 8 Brian Burg 2017-02-16 10:09:25 PST
Created attachment 301777 [details]
Patch v2
Comment 9 Brian Burg 2017-02-16 10:30:43 PST
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.
Comment 10 Brian Burg 2017-02-16 10:31:00 PST
(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.
Comment 11 Joseph Pecoraro 2017-02-16 10:48:50 PST
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.
Comment 12 Brian Burg 2017-02-16 11:03:14 PST
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.
Comment 13 Brian Burg 2017-02-16 11:03:56 PST
WK1 failures do not look related, maybe fallout from import() patch?
Comment 14 Joseph Pecoraro 2017-02-16 11:15:50 PST
(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?)
Comment 15 Brian Burg 2017-02-16 13:33:00 PST
Committed r212462: <http://trac.webkit.org/changeset/212462>