1207506 - [worker debugging] "Services.io is undefined" in /devtools/shared/path.js:13

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[Jan Keromnes [:janx]]

It seems that when trying to debug a worker (launching a Toolbox on a WorkerTarget), we repeatedly hit a "Service.io is undefined" error in path.js:

Full message: TypeError: Services.io is undefined
Full stack: exports.dirname@resource://gre/modules/devtools/shared/path.js:13:3
getSourceURL@resource://gre/modules/devtools/server/actors/script.js:3770:1
SourceActor.prototype.url@resource://gre/modules/devtools/server/actors/script.js:2222:1
SourceActor@resource://gre/modules/devtools/server/actors/script.js:2182:1
TabSources.prototype.source@resource://gre/modules/devtools/server/actors/utils/TabSources.js:154:17
TabSources.prototype.createNonSourceMappedActor@resource://gre/modules/devtools/server/actors/utils/TabSources.js:318:12
TabSources.prototype.createSourceActors/<@resource://gre/modules/devtools/server/actors/utils/TabSources.js:360:19
Handler.prototype.process@resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise-backend.js:813:7
xpcInspector.enterNestedEventLoop@resource://gre/modules/devtools/shared/worker-loader.js:451:9
EventLoop.prototype.enter@resource://gre/modules/devtools/server/actors/script.js:347:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/devtools/server/actors/script.js:544:5
ThreadActor.prototype.onInterrupt@resource://gre/modules/devtools/server/actors/script.js:1403:7
DSC_onPacket@resource://gre/modules/devtools/server/main.js:1596:15
WorkerDebuggerTransport.prototype._onMessage@resource://gre/modules/devtools/shared/transport/transport.js:864:11
EventListener.handleEvent*WorkerDebuggerTransport.prototype.ready@resource://gre/modules/devtools/shared/transport/transport.js:835:9
DS_onConnection@resource://gre/modules/devtools/server/main.js:1116:5
DebuggerServer.connectToParent@resource://gre/modules/devtools/server/main.js:706:12
@resource://gre/modules/devtools/server/worker.js:45:22
EventListener.handleEvent*@resource://gre/modules/devtools/server/worker.js:40:1

Comment 1

[Eddy Bruel [:ejpbruel]]

(In reply to Jan Keromnes [:janx] from comment #0)
> It seems that when trying to debug a worker (launching a Toolbox on a
> WorkerTarget), we repeatedly hit a "Service.io is undefined" error in
> path.js:
> 
> Full message: TypeError: Services.io is undefined
> Full stack:
> exports.dirname@resource://gre/modules/devtools/shared/path.js:13:3
> getSourceURL@resource://gre/modules/devtools/server/actors/script.js:3770:1
> SourceActor.prototype.url@resource://gre/modules/devtools/server/actors/
> script.js:2222:1
> SourceActor@resource://gre/modules/devtools/server/actors/script.js:2182:1
> TabSources.prototype.source@resource://gre/modules/devtools/server/actors/
> utils/TabSources.js:154:17
> TabSources.prototype.createNonSourceMappedActor@resource://gre/modules/
> devtools/server/actors/utils/TabSources.js:318:12
> TabSources.prototype.createSourceActors/<@resource://gre/modules/devtools/
> server/actors/utils/TabSources.js:360:19
> Handler.prototype.process@resource://gre/modules/Promise-backend.js:934:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise-backend.js:813:7
> xpcInspector.enterNestedEventLoop@resource://gre/modules/devtools/shared/
> worker-loader.js:451:9
> EventLoop.prototype.enter@resource://gre/modules/devtools/server/actors/
> script.js:347:5
> ThreadActor.prototype._pushThreadPause@resource://gre/modules/devtools/
> server/actors/script.js:544:5
> ThreadActor.prototype.onInterrupt@resource://gre/modules/devtools/server/
> actors/script.js:1403:7
> DSC_onPacket@resource://gre/modules/devtools/server/main.js:1596:15
> WorkerDebuggerTransport.prototype._onMessage@resource://gre/modules/devtools/
> shared/transport/transport.js:864:11
> EventListener.handleEvent*WorkerDebuggerTransport.prototype.ready@resource://
> gre/modules/devtools/shared/transport/transport.js:835:9
> DS_onConnection@resource://gre/modules/devtools/server/main.js:1116:5
> DebuggerServer.connectToParent@resource://gre/modules/devtools/server/main.
> js:706:12
> @resource://gre/modules/devtools/server/worker.js:45:22
> EventListener.handleEvent*@resource://gre/modules/devtools/server/worker.js:
> 40:1

You can't use Services in a worker, because you can't use XPCOM in a worker. If we are using Services in a worker here, we need to provide an alternative code path.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Moving to debugger, since it's in script actor.

Comment 3

[Eddy Bruel [:ejpbruel]]

I can look into this issue, but I'm going to need some STR.

Comment 4

[Jan Keromnes [:janx]]

STR:

1. Go to about:debugging
2. Go to the "Workers" section
3. Click on any "Other Workers" (e.g. "resource:///modules/sessionstore/SessionWorker.js")

Reproduces 100%.

Comment 5

[Eddy Bruel [:ejpbruel]]

This looks like a regression. The file path.js, which was introduced recently, contains several functions that use Services, which is not available to the worker debugger. That is not at problem in itself, but unfortunately these functions are used by SourceActor, which is used in the worker debugger.

If we want path.js to work with worker debugging, it can't use Services. James, it looks like you wrote this file, so you're probably the most qualified to fix the issue. Could you do me a favor and take a look at it some time soon?

Comment 6

[James Long (:jlongster)]

(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> This looks like a regression. The file path.js, which was introduced
> recently, contains several functions that use Services, which is not
> available to the worker debugger. That is not at problem in itself, but
> unfortunately these functions are used by SourceActor, which is used in the
> worker debugger.
> 
> If we want path.js to work with worker debugging, it can't use Services.
> James, it looks like you wrote this file, so you're probably the most
> qualified to fix the issue. Could you do me a favor and take a look at it
> some time soon?

This file was not introduced recently. You're probably seeing jryans' massive file move, and for some reason I can't look back past that, but it's probably been almost a year since that file's been touched.

Comment 7

[James Long (:jlongster)]

I don't really know what has changed recently if this only started recently. It seems like we should start using non-Services files and maybe start pulling in stuff from npm to do what we need (like url joining).

How did the debugger ever work before?

Comment 8

[James Long (:jlongster)]

File hasn't been touched since sep 1 of last year:

changeset:   263536:3b90d45a2bbc
user:        J. Ryan Stinnett <[e-mail]>
date:        Mon Sep 21 12:02:24 2015 -0500
summary:     Bug 912121 - Migrate major DevTools directories. rs=devtools

changeset:   260289:b2a5bd71898a
user:        Nick Fitzgerald <[e-mail]>
date:        Tue Sep 01 11:25:13 2015 -0700
summary:     NO BUG - s/if(/if (/ in toolkit/devtools JS; r=jimb DONTBUILD

changeset:   219014:f04adec1982d
user:        James Long <[e-mail]>
date:        Tue Dec 09 15:00:00 2014 -0500
summary:     Bug 1107541 - Display eval sources with a sourceURL pragma correctly in the debugger. r=fitzgen

Comment 9

[Jan Keromnes [:janx]]

Thanks for looking into this James. Needinfo Eddy regarding comment 7.

Comment 10

[Brian Grinstead [:bgrins]]

It's possible to use the rpc mechanism to commuicate with the main thread for this (i.e. https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%27%22rpc%22%27&redirect=true&case=false).  However, then it would have to become async.

This path.js file does look like something we might be able to use an existing utility for, although is the services stuff doing any magic for us with resource URIs / jar URIs / data URIs that another utility wouldn't be handling?

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> It's possible to use the rpc mechanism to commuicate with the main thread
> for this (i.e.
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Adevtools+%27%22rpc%22%27&redirect=true&case=false). 
> However, then it would have to become async.
> 
> This path.js file does look like something we might be able to use an
> existing utility for, although is the services stuff doing any magic for us
> with resource URIs / jar URIs / data URIs that another utility wouldn't be
> handling?

As I mentioned during the service worker debugging planning meeting, I'd like to get rid of the rpc mechanism eventually, if we can get away with it. Instead of using rpc, we should either reimplement things in a way that doesn't require Services (fwiw, I think we are heavily overusing Service in the server), or for those cases where we really need platform support, we should extend the worker debugger platform API.

In this particular case, using a solution that does not require Services, as James suggested in comment #7, seems like the right way to go.

Comment 12

[James Long (:jlongster)]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> It's possible to use the rpc mechanism to commuicate with the main thread
> for this (i.e.
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Adevtools+%27%22rpc%22%27&redirect=true&case=false). 
> However, then it would have to become async.
> 
> This path.js file does look like something we might be able to use an
> existing utility for, although is the services stuff doing any magic for us
> with resource URIs / jar URIs / data URIs that another utility wouldn't be
> handling?

I don't think so, it just implements the standard path joining conventions.

I'm still wondering how the debugger ever worked if we are hitting this error now.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > It's possible to use the rpc mechanism to commuicate with the main thread
> > for this (i.e.
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=path%3Adevtools+%27%22rpc%22%27&redirect=true&case=false). 
> > However, then it would have to become async.
> > 
> > This path.js file does look like something we might be able to use an
> > existing utility for, although is the services stuff doing any magic for us
> > with resource URIs / jar URIs / data URIs that another utility wouldn't be
> > handling?
> 
> I don't think so, it just implements the standard path joining conventions.
> 
> I'm still wondering how the debugger ever worked if we are hitting this
> error now.

Weird thing is that it still seems to work for me (at least a separate toolbox debugging a web worker), and I don't see this error show up anywhere in that case.  For instance, on https://bgrins.github.io/devtools-demos/worker/webworker.html.  But it surely seems like it *should* be a problem, so I'm not sure if maybe the error message is being swallowed and things are still working anyway or if this is somehow specific to about:debugging

Comment 14

[Jan Keromnes [:janx]]

[Tracking Requested - why for this release]: Breaks the new Service Worker & Push features that we'd like to announce with Developer Edition 47.

Comment 15

[Jan Keromnes [:janx]]

I did some debugging. When the error happens, here is what the type of some of our globals look like:
 
require:function
importScripts:undefined
Components:object:{}
Services:object:{}
chrome:object:{CC:undefined,Cc:undefined,ChromeWorker:undefined,Cm:undefined,Ci:undefined,Cu:undefined,Cr:undefined,components:undefined}

I suppose we're not inside a worker, because we don't have `importScripts`. However, it's very weird that `Services` or `Components` don't have any keys, and that `chrome`'s keys are all undefined.

I logged this with the following code:

+if (typeof (Services.io) === "undefined") {
+  function stringify(obj) {
+    return (typeof obj) + ":{" + Object.keys(obj).map(key => key + ":" + obj[key]).join(",") + "}";
+  }
+  var chrome = require("chrome");
+  throw new TypeError("\nrequire:" + (typeof require) +
+    "\nimportScripts:" + (typeof importScripts) +
+    "\nComponents:" + stringify(Components) +
+    "\nServices:" + stringify(Services) +
+    "\nchrome:" + stringify(chrome)
+  );
+}

Comment 16

[Jan Keromnes [:janx]]

Apparently "importScripts:undefined" doesn't mean we're not in a worker, and the fact that most things are not defined is symptomatic of a worker.

The question is, why is the worker-loader loading scripts that obviously don't work inside Workers, e.g. devtools/shared/path.js ?

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Jan Keromnes [:janx] from comment #16)
> Apparently "importScripts:undefined" doesn't mean we're not in a worker, and
> the fact that most things are not defined is symptomatic of a worker.
> 
> The question is, why is the worker-loader loading scripts that obviously
> don't work inside Workers, e.g. devtools/shared/path.js ?

Because there is no way in general that the worker loader can tell that a script won't work inside a worker? For instance, script.js contains several references to Services, but it obviously works inside a worker. The reason it does is that we are careful to never hit those code paths inside a worker.

There is a per-module global isWorker, that should be set to true if and only if we're running inside a worker, which is how we implement feature detection for things like Services.

Comment 18

[Eddy Bruel [:ejpbruel]]

Oh! As it turns out, path.js only uses the Services.io.newURI API. This also broke source maps in workers, and I've just written a fix for that, which is about to land: see bug 1119490 for details.

Once that bug lands, we can easily fix this, by replacing all uses of Services.io.newURI with new URL.

Assigning this bug to myself so I can look into it on monday.

Comment 19

[Jan Keromnes [:janx]]

Thanks Eddy! Otherwise we can also just copy/paste `dirname()` and `join()` from toolkit/components/osfile/modules/ospath_unix.jsm

P.S. When logging I see "isWorker:true".

Comment 20

[Eddy Bruel [:ejpbruel]]

Attachment: Replace all uses of Services.io in path.js with URL

This patch replaces all uses of Services.io in path.js with URL. Note that this patch depends on bug 1119490, where we expose the URL constructor to the worker debugger. That bug also contains a test that should cover the same code path that caused this failure, so I didn't write an additional test for this patch.

Comment 21

[Eddy Bruel [:ejpbruel]]

I've opened bug 1250110 to clean up some issues that I discovered with the path functions in the process. See that bug for details.

Comment 22

[Jan Keromnes [:janx]]

Patch looks good, thanks! Trying it now.

Comment 23

[Jan Keromnes [:janx]]

Comment on attachment 8721953 [details] [diff] [review]
Replace all uses of Services.io in path.js with URL

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

Thanks for the patch! It does fix the "Services.io is undefined" problem, but sadly bug 1209559 still reproduces.

Comment 24

[Eddy Bruel [:ejpbruel]]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2537a8e21918

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d53f6871ec13

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d53f6871ec13

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jan, could you please verify this issue is fixed as expected? Thanks!

Comment 28

[Jan Keromnes [:janx]]

Ritu, I confirm this is now fixed \o/ thanks Eddy!

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

Marking the "status-firefox47" flag as verified based on the above comment.