Closed Bug 1207506 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox47+ verified)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 + verified

People

(Reporter: janx, Assigned: ejpbruel)

References

()

Details

Attachments

(1 file)

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
(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.
Moving to debugger, since it's in script actor.
Component: Developer Tools → Developer Tools: Debugger
I can look into this issue, but I'm going to need some STR.
Flags: needinfo?(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%.
Flags: needinfo?(janx)
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?
Assignee: ejpbruel → nobody
Flags: needinfo?(jlong)
(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.
Flags: needinfo?(jlong)
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?
File hasn't been touched since sep 1 of last year:

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

changeset:   260289:b2a5bd71898a
user:        Nick Fitzgerald <fitzgen@gmail.com>
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 <longster@gmail.com>
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
Thanks for looking into this James. Needinfo Eddy regarding comment 7.
Flags: needinfo?(ejpbruel)
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?
(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.
Flags: needinfo?(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?

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.
(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
[Tracking Requested - why for this release]: Breaks the new Service Worker & Push features that we'd like to announce with Developer Edition 47.
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)
+  );
+}
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 ?
(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.
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.
Assignee: nobody → ejpbruel
Depends on: 1119490
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".
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.
Attachment #8721953 - Flags: review?(janx)
Blocks: 1250110
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.
Patch looks good, thanks! Trying it now.
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.
Attachment #8721953 - Flags: review?(janx) → review+
Depends on: 1241841
https://hg.mozilla.org/mozilla-central/rev/d53f6871ec13
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Jan, could you please verify this issue is fixed as expected? Thanks!
Flags: qe-verify+
Flags: needinfo?(janx)
Ritu, I confirm this is now fixed \o/ thanks Eddy!
Status: RESOLVED → VERIFIED
Flags: needinfo?(janx)
Marking the "status-firefox47" flag as verified based on the above comment.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.