Closed Bug 1119490 Opened 9 years ago Closed 8 years ago

Source maps does not work in Web Workers

Categories

(DevTools :: Debugger, defect, P1)

Product:
DevTools
Component:
Debugger
Version:
34 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: iwillig, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

source-map-web-workers.png
26.92 KB, image/png
Details
Enable source maps for workers
1.63 KB, patch
jryans
: review+
Details | Diff | Splinter Review
Expose the URL constructor to WorkerDebuggerGlobalScope.
14.02 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Test for source maps in workers
13.63 KB, patch
jryans
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141201111703

Steps to reproduce:

Write a simple web worker in coffeescript, for example, 

```coffee
throwError = -> throw new Error('hello world')
throwError()
```
Install coffeescript via npm, 

```shell
npm install coffee-script
```

Compile that code via the coffescript compiler with source maps turned on.

```shell
./node_modules/coffee-script/bin/coffee -m -c test.coffee
```

Build a simple html page to load that web worker, 

```html
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title></title>
    <script>
      var worker = new Worker('test.js');
      worker.onerror = function(e) { console.log(e); };
    </script>
  </head>
  <body>
  </body>
</html>

```

And load that page in firefox. 


Actual results:

The error message is correctly logged in the console, but is shows a refrence to the javascript source, not the source map. Attached is an screen shot of the console output. 


Expected results:

Actually I am not sure what _should_ happen. Ideal Firefox would support source maps in web workers. Should maybe this is a feature request not a bug report. 

Please let me know if you need anymore information, or if this is simply not supported and Firefox does not plan on supporting this. 

Thanks.
Component: Untriaged → Developer Tools: Debugger
Currently Firefox does not supporting debugging web workers at all. See bug 1003097 for an overview of the work related in that area.
Blocks: dbg-worker
Status: UNCONFIRMED → NEW
Ever confirmed: true
We now have basic support for debugging web workers.

In theory, source map support for debugging web workers should work out-of-the-box: it uses the same code paths in workers as it does on the main thread. and the same APIs are available on both sides.

That said, we have not specifically tested for this, so it's definitely possible for this not to work. I'm assigning this bug to myself as a reminder to look into this. If source map support does in fact not work in workers, I expect it will be relatively easy to fix.
Assignee: nobody → ejpbruel
(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> We now have basic support for debugging web workers.
> 
> In theory, source map support for debugging web workers should work
> out-of-the-box: it uses the same code paths in workers as it does on the
> main thread. and the same APIs are available on both sides.
> 
> That said, we have not specifically tested for this, so it's definitely
> possible for this not to work. I'm assigning this bug to myself as a
> reminder to look into this. If source map support does in fact not work in
> workers, I expect it will be relatively easy to fix.

IIRC, there were issues with our DevToolsUtils.fetch'ing only working from the main thread. Have we worked those kinks out?
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> (In reply to Eddy Bruel [:ejpbruel] from comment #2)
> > We now have basic support for debugging web workers.
> > 
> > In theory, source map support for debugging web workers should work
> > out-of-the-box: it uses the same code paths in workers as it does on the
> > main thread. and the same APIs are available on both sides.
> > 
> > That said, we have not specifically tested for this, so it's definitely
> > possible for this not to work. I'm assigning this bug to myself as a
> > reminder to look into this. If source map support does in fact not work in
> > workers, I expect it will be relatively easy to fix.
> 
> IIRC, there were issues with our DevToolsUtils.fetch'ing only working from
> the main thread. Have we worked those kinks out?

Yeah, we 'fixed' that problem (i.e. hacked our way around it), so source maps *should* work now. However, we apparently introduced several uses of the Services object in TabSources, and that breaks source handling in workers completely (I've opened bug 1247192). Reminder to self that once that bug is fixed, we should write a test for this, to make sure that source maps do indeed work with worker debugging.

Also note that source maps are currently not supported in the console, neither on the main thread nor in workers, so that's a separate issue.
I've written a patch for bug 1247192 that replaces all uses of the Services object in TabSources with the URL constructor. To make source maps work in workers, all we need to do now is expose the URL constructor to the worker debugger. Here's a patch that does that.
Attachment #8718365 - Flags: review?(khuey)
This patch enables source maps in workers by providing the URL object as a built-in module in the worker loader, and making sure the useSourceMaps option is passed to attachThread in the WorkerClient (we forgot to do so before).
Attachment #8718367 - Flags: review?(jryans)
Comment on attachment 8718367 [details] [diff] [review]
Enable source maps for workers

Review of attachment 8718367 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/client/main.js
@@ +1440,5 @@
>  
>        return this.request({
>          to: connectReponse.threadActor,
> +        type: "attach",
> +        options: aOptions

Could be confusing that aOptions goes into both connect and attach, but up to you.
Attachment #8718367 - Flags: review?(jryans) → review+
Comment on attachment 8718365 [details] [diff] [review]
Expose the URL constructor to WorkerDebuggerGlobalScope.

Review of attachment 8718365 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/RegisterBindings.cpp
@@ +43,5 @@
> +                                        JS::Handle<JSObject*> aGlobal)
> +{
> +  if (!URLBinding_workers::GetConstructorObject(aCx, aGlobal)) {
> +    return false;
> +  }

Why do we need to do this here?  This should get resolved lazily, no?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Comment on attachment 8718365 [details] [diff] [review]
> Expose the URL constructor to WorkerDebuggerGlobalScope.
> 
> Review of attachment 8718365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/RegisterBindings.cpp
> @@ +43,5 @@
> > +                                        JS::Handle<JSObject*> aGlobal)
> > +{
> > +  if (!URLBinding_workers::GetConstructorObject(aCx, aGlobal)) {
> > +    return false;
> > +  }
> 
> Why do we need to do this here?  This should get resolved lazily, no?

I just did the dumb thing and copied this from RegisterWorkerBindings. How do I resolve this lazily? Can you point me to some existing code that does this?
Flags: needinfo?(khuey)
Aha.  For lazy resolving you need something like CGRegisterWorkerBindings in Codegen.py.  Do you want to do that, or should I?
Flags: needinfo?(khuey) → needinfo?(ejpbruel)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Aha.  For lazy resolving you need something like CGRegisterWorkerBindings in
> Codegen.py.  Do you want to do that, or should I?

You're welcome to do it if you want to :-)
Flags: needinfo?(ejpbruel)
Added lazy resolving via CodeGen.py, like you requested.
Attachment #8718365 - Attachment is obsolete: true
Attachment #8718365 - Flags: review?(khuey)
Attachment #8720463 - Flags: review?(khuey)
Comment on attachment 8720463 [details] [diff] [review]
Expose the URL constructor to WorkerDebuggerGlobalScope.

Review of attachment 8720463 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! And here I was looking for time in my schedule to write it.

It doesn't matter for URL, but for other bindings it's not clear to me that permissions checks will do the right thing here. (In particular, the debugger should have chrome privileged APIs even if the worker itself doesn't).  Can you file a follow up on investigating that?
Attachment #8720463 - Flags: review?(khuey) → review+
Try push for exposing the URL constructor to WorkerDebuggerGlobalScope:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e67c30c891bc
Hi James. I know you're on leave for now, so don't feel obliged to respond to this needinfo unless you want to.

I'm trying to write a frontend test for this bug. In particular, I want to write a test in which we load a page that creates a worker with a source-mapped script. To do this, I need to be able to detect when the worker is added to the workers view, so I can then select it. For inspiration, I looked at how we detect when a source is added to the sources view. Unfortunately, the sources view is written in redux, and I can't completely figure it out.

Also, even though sources view has been rewritten in redux, it looks like the workers view hasn't yet, so I'm not sure if it even makes sense to look at the former to see how we should implement the latter. Perhaps I can just emit an event then a new worker is added for now, like we did in the old sources view?
Flags: needinfo?(jlong)
The previous two patches enable source maps in workers, but we don't have any tests for this yet. This patch remedies that.
Attachment #8720887 - Flags: review?(jryans)
(In reply to Eddy Bruel [:ejpbruel] from comment #15)
> Hi James. I know you're on leave for now, so don't feel obliged to respond
> to this needinfo unless you want to.
> 
> I'm trying to write a frontend test for this bug. In particular, I want to
> write a test in which we load a page that creates a worker with a
> source-mapped script. To do this, I need to be able to detect when the
> worker is added to the workers view, so I can then select it. For
> inspiration, I looked at how we detect when a source is added to the sources
> view. Unfortunately, the sources view is written in redux, and I can't
> completely figure it out.
> 
> Also, even though sources view has been rewritten in redux, it looks like
> the workers view hasn't yet, so I'm not sure if it even makes sense to look
> at the former to see how we should implement the latter. Perhaps I can just
> emit an event then a new worker is added for now, like we did in the old
> sources view?

Canceling needinfo for James because I figured it out on my own.
Flags: needinfo?(jlong)
Blocks: 1207506
I'm seeing lots of failures for the try push for exposing the URL constructor to the worker debugger. Some of them seem infrastructure related. At least one is the same GC crash that we've seen before with service worker debugging. I'm not sure what to make of it. The code in this patch doesn't have any consumers yet, so it shouldn't cause any failures.

I'm going to do another try push and see if the failures persist:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eed0cc999ed
That try push looks much more promising. Seeing a lot of timeouts for  dom/indexedDB/test/browser_perwindow_privateBrowsing.js, but that's apparently a known intermittent, and it eventually became green. I've seen the same timeout in earlier try runs, so it's unlikely that these timeouts are caused by my patch.
Priority: -- → P1
Blocks: 1250110
Try push for enabling source maps for workers:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0329274dba9c
Here we go again. This patch causes gc crashes in browser_dbg-worker-window.js on try for no apparent reason. Locally, I've been unable to make the test crash with --run-to-failure.

The last time we ran into this problem, Jan discovered that we were leaking listeners, but we're not using any listener code in this patch. Unless I found a new way to leak things, I'd say there is still something strange going on.

Jan, what do you think? At this point, I am inclined to disable this test. It has prevented me from landing patches for months now. I still cannot reproduce it locally, and the JavaScript folks don't seem to know what's going on either.
Flags: needinfo?(janx)
Unlike previous instances where I ran into gc crashes with this test, this time I am able to reproduce it locally. Could it be that we are leaking listeners (or something similar) with this patch?
Depends on: 1241841
https://hg.mozilla.org/mozilla-central/rev/758daaf1723f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave-open]
As discussed on IRC, this crash was fixed by cleaning up all the queued runnables of a worker on shutdown. Hurray! \o/
Flags: needinfo?(janx)
New try push for enabling source maps in workers:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e2f2ca10f04
Try push had failing xpcshell tests because they try to use the worker loader on the main thread instead of in a worker, and the URL constructor is not defined for xpcshell tests.

Here's a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b83af0202e1
Try push for testing source maps in workers:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9daf8d02d30e
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e11ee5db95dd
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: