Bug 217835 – Web Inspector: move InspectorFrontendAPIDispatcher to WebCore, clean up uses

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Splitting this off from 217783.

[Radar WebKit Bug Importer]

<rdar://problem/70384407>

[Blaze Burg]

Created attachment 411613 [details]
Patch v1

This will probably take some iteration with EWS for non-Cocoa ports.

[Devin Rousso]

Comment on attachment 411613 [details]
Patch v1

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

r=me this looks fine to me, so I'm gonna preemptively r+ on the assumption that you'll fix build/test failures :)

please let me know if anything significant changes while doing so and I can take another look

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:45
> +    void dispatchCommand(const String& command, Vector<Ref<JSON::Value>>&& arguments);

How would you feel about using a different name?  Maybe something like `invoke` or `evaluate`?  Using "command" makes me thing this is related to protocol commands (which I think go through `dispatchMessageAsync`).

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:396
>  bool InspectorFrontendClientLocal::evaluateAsBoolean(const String& expression)

Why not also move this to `InspectorFrontendAPIDispatcher::evaluateAsBoolean` (or with a different/better name)?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:-433
> -    JSC::SuspendExceptionScope scope(&m_frontendPage->inspectorController().vm());

Do we still need this somewhere?

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:75
>      StringBuilder builder;
>      JSON::Value::escapeString(builder, findString);

I think we can drop this now because you added a `writeJSON` call, which calls `escapeString`.

Actually, keeping this may result in over-escaping 😳

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.h:95
> +    WebCore::InspectorFrontendAPIDispatcher& frontendAPIDispatcher() final { return m_frontendAPIDispatcher; }

NIT: the `final` isn't necessary since this class is already `final`

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:247
> +    m_frontendAPIDispatcher.dispatchCommand("setDockSide"_s, { JSON::Value::create(String(dockSideString)) });

NIT: is the `String` necessary?

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:267
>      StringBuilder builder;
>      JSON::Value::escapeString(builder, findString);

ditto (RemoteWebInspectorUI.cpp:74)

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:334
> +    m_frontendAPIDispatcher.dispatchCommand("showConsole"_s, { });

you can add `= { }` to the declaration of `dispatchCommand` to avoid having to do this at callsites

[Joseph Pecoraro]

Comment on attachment 411613 [details]
Patch v1

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

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:78
> +    expression.appendLiteral("\"");

I'd love to see this just be `append('"')`!

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:91
> +    evaluateOrQueueExpression(makeString("InspectorFrontendAPI.dispatchMessageAsync(", message, ")"));

Likewise ")" => ')'

[Blaze Burg]

Created attachment 412135 [details]
Patch (with debug logging)

[Blaze Burg]

Upon investigation, some evaluations are getting stuck because we suspend as the inspected page (i.e, also the inspector frontend page) pauses.

Prior to this patch, pagePaused() did not call suspend(), so I think for WK1 purposes we have to be okay with evaluating message dispatches in the frontend page while it is paused.

This seems gross and we should really get rid of this in-process stuff altogether.

[Blaze Burg]

Created attachment 412587 [details]
InspectorFrontendAPIDispatcher protocol logging w/ backtrace

[Blaze Burg]

(In reply to Brian Burg from comment #6)
> Upon investigation, some evaluations are getting stuck because we suspend as
> the inspected page (i.e, also the inspector frontend page) pauses.
> 
> Prior to this patch, pagePaused() did not call suspend(), so I think for WK1
> purposes we have to be okay with evaluating message dispatches in the
> frontend page while it is paused.
> 
> This seems gross and we should really get rid of this in-process stuff
> altogether.

I fixed the suspend/unsuspend behavior and related assertions. It still seems like the frontend is getting hung / evaluations aren't happening:

Test code:

 InspectorProtocol.sendCommand("Runtime.evaluate", {expression: "this.should.trigger.an.exception"}, function(messageObject) { ...

Logging:

PASS: Paused in debugger: reason = "Breakpoint"

BJ>> Okay, it got to onBreakpointHit and its sending the Runtime.evaluate command on the bad expression

frontend: {"method":"Runtime.evaluate","params":{"expression":"TestPage.log(unescape(\"PASS%3A%20Paused%20in%20debugger%3A%20reason%20%3D%20%22Breakpoint%22\"));"},"id":6}
frontend: {"method":"Runtime.evaluate","params":{"expression":"this.should.trigger.an.exception"},"id":7}
FrontendAPIDispatcher[0x351c7fa78, queue depth=0] evaluating: InspectorFrontendAPI.dispatchMessageAsync({"method":"Debugger.scriptParsed","params":{"scriptId":"9","url":"","startLine":0,"startColumn":0,"endLine":0,"endColumn":96,"isContentScript":false,"module":false}})

BJ>> Hmm, we should see 'backend: <the same message>' being logged for every InspectorFrontendAPI.dispatchMessageAsync, but from here onwards I don't see it:

FrontendAPIDispatcher[0x351c7fa78, queue depth=0] evaluating: InspectorFrontendAPI.dispatchMessageAsync({"result":{"result":{"type":"undefined"},"wasThrown":false},"id":6})
FrontendAPIDispatcher[0x351c7fa78, queue depth=0] evaluating: InspectorFrontendAPI.dispatchMessageAsync({"method":"Debugger.scriptParsed","params":{"scriptId":"10","url":"","startLine":0,"startColumn":0,"endLine":0,"endColumn":32,"isContentScript":false,"module":false}})
FrontendAPIDispatcher[0x351c7fa78, queue depth=0] evaluating: InspectorFrontendAPI.dispatchMessageAsync({"result":{"result":{"type":"object","objectId":"{\"injectedScriptId\":1,\"id\":65}","subtype":"error","className":"TypeError","description":"TypeError: undefined is not an object (evaluating 'this.should.trigger')"},"wasThrown":true},"id":7})

BJ>> At this point, three messages have been 'evaluated' with nothing actually happening. Hmm.. Maybe I messed up the evaluation worlds?

[Blaze Burg]

Created attachment 412672 [details]
Patch v2.1 (for EWS)

[Devin Rousso]

Comment on attachment 412672 [details]
Patch v2.1 (for EWS)

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

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:75
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, { JSON::Value::create(builder.toString()) });

btw this should use `findString` (kinda shocked that this isn't throwing an error about "unused variable `findString" 🤔) and drop `builder` altogether

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:267
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, { JSON::Value::create(builder.toString()) });

ditto (RemoteWebInspectorUI.cpp:75)

[Blaze Burg]

Created attachment 412692 [details]
Patch v2.2 - fix some protocol test bugs

This patch addresses all known issues, want to see what the bots do.

[Blaze Burg]

Created attachment 412716 [details]
Patch v2.3 - for EWS

[Blaze Burg]

Created attachment 412761 [details]
Patch v2.4 - for EWS

[Devin Rousso]

Comment on attachment 412761 [details]
Patch v2.4 - for EWS

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

r=me, with two fixes for `updateFindString` (I still don't know how that's not throwing a compiler error because of the unused variable o_O)

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:85
> +            if (!protectedThis->isSuspended())
> +                return;

NIT: this check seems unnecessary since it's repeated in `unsuspend`

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:65
> +    EvaluationResult dispatchCommandSync(const String& command, Vector<Ref<JSON::Value>>&& arguments = { });

NIT: wouldn't it be more accurate to call this `dispatchCommandWithResultSync`?

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:66
> +    WEBCORE_EXPORT void dispatchCommandWithResult(const String& command, Vector<Ref<JSON::Value>>&& arguments, EvaluationResultHandler&&);

Seeing as `dispatchCommand` and `dispatchCommandWithResult` basically have the same logic, could we combine them and avoid the repeated logic?
```
    EvaluationResult dispatchCommandWithResultSync(const String& command, Vector<Ref<JSON::Value>>&& arguments = { });
    WEBCORE_EXPORT void dispatchCommandWithResultAsync(const String& command, Vector<Ref<JSON::Value>>&& arguments = { }, EvaluationResultHandler&& = { });
```

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:79
> +    void evaluateOrQueueExpression(const String&, WTF::Optional<EvaluationResultHandler>&& = WTF::nullopt);

it shouldn't be necessary to use `Optional` as `CompletionHandler` already has a way to check "not set"

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:85
> +    Vector<std::pair<String, Optional<EvaluationResultHandler>>> m_queuedEvaluations;

ditto (:79)

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:-188
> -    m_frontendLoaded = true;

Don't we still need this?  It's still used later.

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:331
> +    auto result = m_frontendAPIDispatcher->dispatchCommandSync("isDebuggingEnabled"_s, { });

NIT: don't need the `{ }`

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:348
> +    auto result = m_frontendAPIDispatcher->dispatchCommandSync("isTimelineProfilingEnabled"_s, { });

ditto (:331)

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:369
> +    if (!result || !result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject()))
> +        return false;
> +
> +    return true;

NIT: you could simplify this
```
    return result && result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
```

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:75
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, { JSON::Value::create(builder.toString()) });

this should use `findString`, not `builder`

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:267
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, { JSON::Value::create(builder.toString()) });

this should use `findString`, not `builder`

> LayoutTests/inspector/dom/dom-remove-events.html:74
> +            delete InspectorProtocol.eventHandler["DOM.setChildNodes"];

I wonder if we should just change `ProtocolTest.completeTest` to automatically nuke `InspectorProtocol.eventHandler` and dispatch `disable` commands to every domain 🤔

[Blaze Burg]

Comment on attachment 412761 [details]
Patch v2.4 - for EWS

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

>> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:85
>> +                return;
> 
> NIT: this check seems unnecessary since it's repeated in `unsuspend`

Ah right. I think it was added when unsuspend() was more strict and asserted a value change.

>> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:79
>> +    void evaluateOrQueueExpression(const String&, WTF::Optional<EvaluationResultHandler>&& = WTF::nullopt);
> 
> it shouldn't be necessary to use `Optional` as `CompletionHandler` already has a way to check "not set"

Oh. :til:

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:-188
>> -    m_frontendLoaded = true;
> 
> Don't we still need this?  It's still used later.

It's only used to guard calls to isProfilingEnabled and the like. Since frontendAPIDispatcher will check for m_frontendLoaded and return an error if it's not loaded, I think this can be removed entirely.

>> LayoutTests/inspector/dom/dom-remove-events.html:74
>> +            delete InspectorProtocol.eventHandler["DOM.setChildNodes"];
> 
> I wonder if we should just change `ProtocolTest.completeTest` to automatically nuke `InspectorProtocol.eventHandler` and dispatch `disable` commands to every domain 🤔

It would be better to nuke Protocol tests altogether, but alas, I have no more time to fiddle with old tests.

[Blaze Burg]

Created attachment 412780 [details]
Patch v2.5 - for EWS

[Blaze Burg]

Created attachment 412793 [details]
Patch v2.6 - for EWS

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Devin Rousso]

Comment on attachment 412793 [details]
Patch v2.6 - for EWS

r=me

[EWS]

Committed r269210: <https://trac.webkit.org/changeset/269210>

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