Bug 180520 – ServiceWorker Inspector: Various issues inspecting service worker on mobile.twitter.com

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Various issues inspecting service worker on mobile.twitter.com

  1. Exceptions with SourceMaps, opening resources via URLs, and other places around the frontend
  2. Issues with Failed Resources not showing up / throwing exceptions
  3. Issues with Main resource not showing up (Script without function gets thrown away, so we need to implant a fake one)

[Radar WebKit Bug Importer]

<rdar://problem/35900764>

[Joseph Pecoraro]

Created attachment 328678 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 328678 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/protocol/ServiceWorker.json:14
> +                { "name": "content", "type": "string", "description": "ServiceWorker main script content." }

I don't exactly like this. But this is the situation that we are running into:

The Service Worker script content was requested outside of the ServiceWorker Process. So we don't have a traditional "Resource". Normally for Workers that is fine, we get a "Script" from ScriptDebugServer when Debugger.enable, since the Worker Script evaluated in the VM. However the JSC::VM is free to throw that script content away if it doesn't need it anymore (e.g. if there are no function's in the source code that it needs to reference later on).

So possible solutions are:

1. We have the content for the main resource, so send it along with the configuration data.
  - Pro: easy
  - Con: if the content is huge its a waste and will probably be replaced by a Script later on

2. Keep the SW MainResource content on the backend, make a Resource in the frontend that knows to request it from the ServiceWorker domain
  - Pro: No big message sent to the frontend
  - Con: Yet another special kind of backend backed resource (Network.getResponseBody(requestId), Debugger.getScriptSource(scriptId), ServiceWorker.getMainResourceContent())

I haven't fully through through the alternatives, but this one gets us working.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:653
> +        InspectorBackend.runAfterPendingDispatches(() => {
> +            if (WI.mainTarget.mainResource === script) {

And this is where it gets disgusting. If we get a Debugger.scriptParsed for this URL we'll end up using that WI.Script. But if we didn't, then we fall back to the LocalScript here so that some main resource shows up in the UI. This LocalScript is lame (can't set breakpoints) but at least we have its contents.

[EWS Watchlist]

Comment on attachment 328678 [details]
[PATCH] Proposed Fix

Attachment 328678 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5526026

New failing tests:
js/dom/modules/import-execution-order.html

[EWS Watchlist]

Created attachment 328683 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Blaze Burg]

Comment on attachment 328678 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:647
> +        // Create a main resource with this content in case the content never shows up as a WI.Script.

Please rename the containing method to _processServiceWorkerConfiguration since you renamed the type in this patch. This comment may apply elsewhere.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:655
> +                console.log("USING LOCAL RESOURCE");

Nit: Please remove

> Source/WebInspectorUI/UserInterface/Models/TextRange.js:59
> +        return new WI.TextRange(0, 0, lines.length - 1, lines.lastValue.length);

Is the end column not also zero-based? I would have expected lines.lastValue.length - 1.

[Joseph Pecoraro]

Comment on attachment 328678 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:647
>> +        // Create a main resource with this content in case the content never shows up as a WI.Script.
> 
> Please rename the containing method to _processServiceWorkerConfiguration since you renamed the type in this patch. This comment may apply elsewhere.

Good idea, done.

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:655
>> +                console.log("USING LOCAL RESOURCE");
> 
> Nit: Please remove

Oops!

>> Source/WebInspectorUI/UserInterface/Models/TextRange.js:59
>> +        return new WI.TextRange(0, 0, lines.length - 1, lines.lastValue.length);
> 
> Is the end column not also zero-based? I would have expected lines.lastValue.length - 1.

So I based this around:

    "a\nb\n"

Should have:

    startLine: 0
    startColumn: 0
    endLine: 3
    endColumn: 0

And:

    "a\nb\nc"

Should have:

    startLine: 0
    startColumn: 0
    endLine: 3
    endColumn: 1

---

Which makes it obvious now that I should have added tests...

[Joseph Pecoraro]

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