Bug 179043 – Make ServiceWorker a Remote Inspector debuggable target

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Make ServiceWorker a Remote Inspector debuggable target

We should have the ability to directly inspect a Service Worker.

A Service Worker's capabilities lies in between a JavaScript Context debuggable and a Web Page debuggable.

  • JavaScript Context has: Script capabilities
  • ServiceWorker has: Script capabilities, Network capabilities
  • Web page has: Script capabilities, Network capabilities, DOM capabilities

Also JavaScript is expected to evaluating inside of a Service Worker on the specific Worker Context Thread. Evaluating on arbitrary threads (JSContext's normal remote inspection semantics) don't gel with worker's semantics.

[Joseph Pecoraro]

<rdar://problem/34126008>

[Joseph Pecoraro]

Created attachment 325416 [details]
[IMAGE] ServiceWorker Inspector window

[Joseph Pecoraro]

Created attachment 325417 [details]
[PATCH] Proposed Fix - Part 1 (Backend)

[Joseph Pecoraro]

Created attachment 325418 [details]
[PATCH] Proposed Fix - Part 2 (Frontend)

[Build Bot]

Attachment 325417 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 325417 [details]
[PATCH] Proposed Fix - Part 1 (Backend)

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

> Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:54
> +    enum class Type { JavaScript, ServiceWorker, Web, Automation };

I didn't wrap any of this code in ENABLE(SERVICE_WORKER) but I suppose I could. It would cramp the style up in a few places but otherwise is straightforward.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:189
> +    // FIXME: Support remote debugging of a ServiceWorker.

I can file a bugzilla bug on GTK before landing.

[youenn fablet]

Comment on attachment 325417 [details]
[PATCH] Proposed Fix - Part 1 (Backend)

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

> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:45
> +void ServiceWorkerDebuggable::connect(Inspector::FrontendChannel* channel, bool, bool)

FrontendChannel&... in the future probably.

> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:50
> +void ServiceWorkerDebuggable::disconnect(Inspector::FrontendChannel* channel)

Ditto.

> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.h:46
> +    Inspector::RemoteControllableTarget::Type type() const override { return Inspector::RemoteControllableTarget::Type::ServiceWorker; }

override -> final and probably could be made private.

> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.h:49
> +    String url() const override { return m_scriptURL; }

const String&

> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp:49
> +    ASSERT(!m_channel);

Should we also assert it is deleted on the main thread?

> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:47
> +    explicit ServiceWorkerInspectorProxy();

Why explicit?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:65
> +    m_remoteDebuggable = std::make_unique<ServiceWorkerDebuggable>(*this, data);

Do we need to create it now? Can we do that lazily?
If so, maybe use makeUniqueRef.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:72
> +    m_inspectorProxy.serviceWorkerStarted(m_serviceWorkerThread);

Same for m_inspectorProxy, can it be lazily created as an optional?
Also, why does it need to keep a ref to m_serviceWorkerThread?

[Joseph Pecoraro]

Comment on attachment 325417 [details]
[PATCH] Proposed Fix - Part 1 (Backend)

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

>> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp:49
>> +    ASSERT(!m_channel);
> 
> Should we also assert it is deleted on the main thread?

Sure. This will be deleted with the ServiceWorkerThreadProxy, which right now I don't think it ever gets deleted.

>> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:47
>> +    explicit ServiceWorkerInspectorProxy();
> 
> Why explicit?

Oops, I'll remove.

>> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:65
>> +    m_remoteDebuggable = std::make_unique<ServiceWorkerDebuggable>(*this, data);
> 
> Do we need to create it now? Can we do that lazily?
> If so, maybe use makeUniqueRef.

The debuggable should not be lazy, this is what makes the ServiceWorker appear to remote inspector debuggers.

>> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:72
>> +    m_inspectorProxy.serviceWorkerStarted(m_serviceWorkerThread);
> 
> Same for m_inspectorProxy, can it be lazily created as an optional?
> Also, why does it need to keep a ref to m_serviceWorkerThread?

After talking with Youenn I'll make this take a ServiceWorkerThreadProxy& and only access the ServiceWorkerThread through the proxy. Technically the ServiceWorkerThreadProxy ServiceWorkerInspectorProxy can be the same object, I just split it out (like we do in DedicatedWorkers) to keep the inspector hooks / logic bundled together.

[Blaze Burg]

Comment on attachment 325417 [details]
[PATCH] Proposed Fix - Part 1 (Backend)

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

LGTM, but let's see another patch if you are going to adopt ServiceWorkerThreadProxy.

>> Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:54
>> +    enum class Type { JavaScript, ServiceWorker, Web, Automation };
> 
> I didn't wrap any of this code in ENABLE(SERVICE_WORKER) but I suppose I could. It would cramp the style up in a few places but otherwise is straightforward.

I wouldn't bother. We aren't paying a tax to add a new enum value. The only thing I would be careful of is relying on symbols that are wrapped in the ENABLE macro.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:189
>> +    // FIXME: Support remote debugging of a ServiceWorker.
> 
> I can file a bugzilla bug on GTK before landing.

Please.

> Source/WebCore/Sources.txt:2169
> +workers/service/context/ServiceWorkerDebuggable.cpp

How do these files work? Are we supposed to add cpp or .h files to both Xcode and Sources.txt?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10706
> +		A52B348C1FA3BD79008B6246 /* ServiceWorkerDebuggable.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ServiceWorkerDebuggable.h; sourceTree = "<group>"; };

Ditto. Or is this just for the navigator groups?

>> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:45
>> +void ServiceWorkerDebuggable::connect(Inspector::FrontendChannel* channel, bool, bool)
> 
> FrontendChannel&... in the future probably.

Yeah, this is a cleanup that we can do.

> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:33
> +// Used to send messages to the WorkerInspector on the WorkerThread.

Nit: ServiceWorkerThread and ServiceWorkerInspector[Proxy].

> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:54
> +    void disconnectFromWorkerInspectorController(Inspector::FrontendChannel*);

Could these drop the InspectorController suffix? Doesn't seem to add anything.

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:73
> +    : WorkerThread(data.scriptURL, data.workerID, ASCIILiteral("WorkerUserAgent"), data.script, loaderProxy, debuggerProxy, DummyServiceWorkerThreadProxy::shared(), WorkerThreadStartMode::Normal, ContentSecurityPolicyResponseHeaders { }, false, SecurityOrigin::create(data.scriptURL).get(), MonotonicTime::now(), nullptr, nullptr, JSC::RuntimeFlags::createAllEnabled(), SessionID::defaultSessionID())

Holy cow, how so many arguments!?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:92
> +        // FIXME: Handle terminated case.

File a bug!

[Blaze Burg]

Comment on attachment 325418 [details]
[PATCH] Proposed Fix - Part 2 (Frontend)

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

Nice cleanup. r=me

> Source/WebInspectorUI/ChangeLog:9
> +        Customize a the Web Inspector window for a ServiceWorker target.

Nit: -a

> Source/WebInspectorUI/ChangeLog:11
> +        we haven't yet added support for Networking capabitilites.

Nit: capabilities

[Joseph Pecoraro]

Created attachment 325791 [details]
[PATCH] For Landing

[Joseph Pecoraro]

<https://trac.webkit.org/r224368>