Service Worker is no longer inspectable
<rdar://problem/72883757>
Seems to be related with https://bugs.webkit.org/show_bug.cgi?id=219386
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?
Created attachment 417254 [details] Patch
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.
(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?
(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.
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.
Created attachment 417263 [details] Fixes the service worker regression
(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?
(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.
(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.
Created attachment 417264 [details] Patch
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.
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.
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.
Committed r271294: <https://trac.webkit.org/changeset/271294> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417264 [details].