Bug 220406 – Service Worker is no longer inspectable

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Service Worker is no longer inspectable

[youenn fablet]

<rdar://problem/72883757>

[youenn fablet]

Seems to be related with https://bugs.webkit.org/show_bug.cgi?id=219386

[youenn fablet]

ServiceWorkerDebuggable (which implements Inspector::RemoteInspectionTarget) is currently owned by ServiceWorkerThreadProxy which lives in WebContent process.
There is no other debuggable in UIProcess contrary to Page and WebPageProxy.
Maybe this is what is missing?

[Per Arne Vollan]

Created attachment 417254 [details]
Patch

[youenn fablet]

Comment on attachment 417254 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
> +    if (WebCore::applicationBundleIdentifier() == "com.apple.Safari" && !CFPreferencesGetAppIntegerValue(CFSTR("ShowDevelopMenu"), CFSTR("com.apple.Safari.SandboxBroker"), nullptr))

This additional check is a no-op in case the app is Safari and Safari is no longer able to inspect service workers.
It is not clear to me how this could fix the issue.

[Per Arne Vollan]

(In reply to youenn fablet from comment #5)
> Comment on attachment 417254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417254&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
> > +    if (WebCore::applicationBundleIdentifier() == "com.apple.Safari" && !CFPreferencesGetAppIntegerValue(CFSTR("ShowDevelopMenu"), CFSTR("com.apple.Safari.SandboxBroker"), nullptr))
> 
> This additional check is a no-op in case the app is Safari and Safari is no
> longer able to inspect service workers.
> It is not clear to me how this could fix the issue.

I think Safari is still able to inspect service workers, but Safari Technology Preview is not, or perhaps I am mistaken?

[youenn fablet]

(In reply to Per Arne Vollan from comment #6)
> (In reply to youenn fablet from comment #5)
> > Comment on attachment 417254 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417254&action=review
> > 
> > > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
> > > +    if (WebCore::applicationBundleIdentifier() == "com.apple.Safari" && !CFPreferencesGetAppIntegerValue(CFSTR("ShowDevelopMenu"), CFSTR("com.apple.Safari.SandboxBroker"), nullptr))
> > 
> > This additional check is a no-op in case the app is Safari and Safari is no
> > longer able to inspect service workers.
> > It is not clear to me how this could fix the issue.
> 
> I think Safari is still able to inspect service workers, but Safari
> Technology Preview is not, or perhaps I am mistaken?

I tried Safari with the latest WebKit ToT and it no longer sees service worker.
When reverting https://trac.webkit.org/changeset/270326, I can see airhorner.com.

[youenn fablet]

I locally did a test to pass the sandbox extension at the time we initialise the web process and it fixes the issue.
It seems the sandbox extension arrives too late.

[youenn fablet]

Created attachment 417263 [details]
Fixes the service worker regression

[Per Arne Vollan]

(In reply to youenn fablet from comment #8)
> I locally did a test to pass the sandbox extension at the time we initialise
> the web process and it fixes the issue.
> It seems the sandbox extension arrives too late.

Ah, good catch! Maybe we need to call enableRemoteInspectorIfNeeded() after having intialized the web process?

[Per Arne Vollan]

(In reply to youenn fablet from comment #9)
> Created attachment 417263 [details]
> Fixes the service worker regression

It seems this patch creates the extension unconditionally. I think we only should create the extension when the Develop menu is enabled.

[youenn fablet]

(In reply to Per Arne Vollan from comment #11)
> (In reply to youenn fablet from comment #9)
> > Created attachment 417263 [details]
> > Fixes the service worker regression
> 
> It seems this patch creates the extension unconditionally. I think we only
> should create the extension when the Develop menu is enabled.

Right, and it should not be MAC specific, I just wrote it to validate the assumption that the sandbox extension was arriving too late.
I am not sure why it is important to happen sooner rather than later, it would be interesting to understand why.

I guess we could still use this patch approach as a fix.

[youenn fablet]

Created attachment 417264 [details]
Patch

[youenn fablet]

This patch fixes the main regression.
The case that does not work is if the process running the service worker started while the develop menu is off.
I think additional refactoring should be done to fix that case.

[Per Arne Vollan]

Comment on attachment 417264 [details]
Patch

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

Great, thanks for fixing! R=me.

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:225
> +void WebProcessProxy::enableRemoteInspectorIfNeeded()
> +{
> +    if (!shouldEnableRemoteInspector())
> +        return;
> +    send(Messages::WebProcess::EnableRemoteWebInspector(), 0);

It this function still needed? It could be useful when we implement observation of the Develop menu preference key.

[Per Arne Vollan]

Comment on attachment 417264 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:225
>> +    send(Messages::WebProcess::EnableRemoteWebInspector(), 0);
> 
> It this function still needed? It could be useful when we implement observation of the Develop menu preference key.

I see that it is still needed; we call it when receiving an enable remote inspector notification.

[EWS]

Committed r271294: <https://trac.webkit.org/changeset/271294>

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