Closed Bug 859372 Opened 11 years ago Closed 6 years ago

Use devtools loader for the debugger server

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: ejpbruel)

References

Details

Attachments

(12 files, 26 obsolete files)

126.84 KB, patch
Details | Diff | Splinter Review
21.32 KB, patch
past
: review+
Details | Diff | Splinter Review
4.15 KB, patch
past
: review+
Details | Diff | Splinter Review
5.18 KB, patch
past
: review+
Details | Diff | Splinter Review
17.14 KB, patch
past
: review+
Details | Diff | Splinter Review
3.31 KB, patch
past
: review+
Details | Diff | Splinter Review
12.98 KB, patch
past
: review+
Details | Diff | Splinter Review
20.06 KB, patch
past
: review+
Details | Diff | Splinter Review
6.61 KB, patch
past
: review+
Details | Diff | Splinter Review
928 bytes, patch
past
: review+
Details | Diff | Splinter Review
10.84 KB, patch
past
: review+
Details | Diff | Splinter Review
3.56 KB, patch
past
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch WIP 1 (obsolete) — Splinter Review
Summary: Use jetpack loader for debugger server → Use devtools loader for the debugger server
Attached patch WIP 2Splinter Review
Attachment #743717 - Attachment is obsolete: true
Blocks: 850019
Blocks: metrobacklog
Whiteboard: [feature] p=0
This is blocking bug 757133. We don't have Components in workers, so we can use neither Cu.import nor loadSubScript if we want the debugger server to run there.

We could cut some corners, and expose import and loadSubScript directly on the debugger's global inside a worker. However, doing so would still require us to adapt all callers to use this.import instead of Cu.import when running inside a worker (and idem for loadSubScript), which is a pain in the ass. I feel that doing so would make an already messy situation even worse, and once the worker debugger stuff lands, it will be that much harder to refactor.

The right thing to do would, in my mind at least, be to expose a Sandbox like API on the worker debugger global, and use that to define a module loader that we can use inside workers. The patch in bug 757133 already implements such an API.  However, this solution will only work if we replace all references to Cu.import and loadSubScript in the server with require. Since that's apparently already what we're trying to do anyway, this might be the perfect time to do it.

dcamp and mossop, are you ok with me spending some time on this?
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(dcamp)
Absolutely!
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(dcamp)
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
This patch makes it possible to require DevToolsUtils.js, and substitutes all calls to Cu.import and loadSubScript for this file with calls to require in the server.

Note that tracer.js doesn't need the call to Cu.import at all, because it currently shares a global with main.js, which already has reportException defined. This will change once we require tracer.js within its own global, of course.

I've kept DevToolsUtils.jsm around for now, since other files outside the server depend on it as well.

In general, for the sake of understandability, I think that instead of requiring single functions such as reportExceptions, we should perhaps namespace them like so: DevToolsUtils.reportExceptions. This makes it immediately clear where a name cames from. How do you feel about this, Panos?

I'm currently not able to test these patches on B2G, as I don't have a B2G testing device. Panos, would you be so kind as to test these patches on B2G for me, until I can get my hands on such a device?
Attachment #8366683 - Flags: review?(past)
Assignee: nobody → ejpbruel
Blocks: 757133
Attached patch Make Services requirable (obsolete) — Splinter Review
This patch adds Services as a predefined module to the loader, and removes all references to Cu.import for this module in favor of require in the server.

Since Services is used to access most XPCOM objects used by the debugger server, it makes sense to isolate it within the loader, since workers will get their own custom loader.

Note that adding Services as a module freezes the object, which breaks lazy module getting. To work around this (without pulling in any dependencies on add-on SDK code) I've made Services a proxy that forwards to the actual cache object.

I ran into a very nasty bug caused by the fact that gcli uses its own require, which interfered with the version of require used by the loader. I've renamed the former to gcliRequire to work around this.
Attachment #8367039 - Flags: review?(past)
Testing on B2G produces these errors:

I/GeckoDump( 3459): Error while initializing devtools: ReferenceError: DebuggerServer is not defined
I/GeckoDump( 3459): debugger_start@chrome://browser/content/shell.js:1079
I/GeckoDump( 3459): @chrome://browser/content/settings.js:473
I/GeckoDump( 3459): sl_onchange@chrome://browser/content/settings.js:42

You need to update b2g/chrome/content/shell.js and possibly b2g/chrome/content/dbg-browser-actors.js.For Fennec you need to update mobile/android/chrome/content/browser.js and maybe even dbg-browser-actors.js. Let me know if you need me to test on Fennec as well.

Testing the patches on Linux I see problems with chrome debugging, have you tried that? Opening the Browser Toolbox gives me a debugger that often fails to load sources, a console that has issues with the object inspector popup and a style editor that doesn't load stylesheets.

I haven't looked at the code yet.
(In reply to Panos Astithas [:past] from comment #7)
> Testing on B2G produces these errors:
> 
> I/GeckoDump( 3459): Error while initializing devtools: ReferenceError:
> DebuggerServer is not defined
> I/GeckoDump( 3459): debugger_start@chrome://browser/content/shell.js:1079
> I/GeckoDump( 3459): @chrome://browser/content/settings.js:473
> I/GeckoDump( 3459): sl_onchange@chrome://browser/content/settings.js:42
> 
> You need to update b2g/chrome/content/shell.js and possibly
> b2g/chrome/content/dbg-browser-actors.js.For Fennec you need to update
> mobile/android/chrome/content/browser.js and maybe even
> dbg-browser-actors.js. Let me know if you need me to test on Fennec as well.
> 
> Testing the patches on Linux I see problems with chrome debugging, have you
> tried that? Opening the Browser Toolbox gives me a debugger that often fails
> to load sources, a console that has issues with the object inspector popup
> and a style editor that doesn't load stylesheets.
> 
> I haven't looked at the code yet.

Is this for both patches or just the first?

It's a bit worrying that you can find so much failures with my patch even though all the test seem to run just fine.
Attached patch Make DevToolsUtils.js requirable (obsolete) — Splinter Review
I've resolved the issues with chrome debugging by giving the debugger server its own custom loader, with invisibleToDebugger set to true. This ensures that all debugger server globals are invisible to the debugger.

I'm unsure what I have to change to make this patch work on Fennec or B2G (or indeed, if any changes are needed at all, since you only tested with *both* patches applied so far). If the changes are simple, perhaps you could help me out?
Attachment #8366683 - Attachment is obsolete: true
Attachment #8366683 - Flags: review?(past)
Attachment #8367371 - Flags: review?(past)
I'm getting this error when toggling pretty-printing in the debugger:

togglePrettyPrint threw an exception: TypeError: this.makeInfallible is not a function
Stack: executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:119
loop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:151
yieldingEach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:142
SourceActor.prototype._invertSourceMap@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:2569
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:185
SourceActor.prototype._sendToPrettyPrintWorker/</onReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:2543
Line: 119, column: 0
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
Updated patch with fixes for several failing tests. The pretty printing bug should also be fixed in this version.

The problem I'm running into right now is that browser/devtools/fontinspector/test/browser_fontinspector.js never finishes. Apparently there is some problem during initialization, but I can't figure out where.
Attachment #8367371 - Attachment is obsolete: true
Attachment #8367371 - Flags: review?(past)
Attachment #8367843 - Flags: review?(past)
Comment on attachment 8367843 [details] [diff] [review]
Refactor DevToolsUtils.js

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

I took a look at the patch and here are my comments, while still trying to figure out the test failures and try it on b2g & fennec.

::: toolkit/devtools/server/actors/tracer.js
@@ -6,5 @@
>  
>  const { Cu } = require("chrome");
> -
> -const { reportException } =
> -  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {}).DevToolsUtils;

If you remove this, you have to modify all uses of reportException in this file.

::: toolkit/devtools/server/main.js
@@ +9,5 @@
>   * Toolkit glue for the remote debugging protocol, loaded into the
>   * debugging global.
>   */
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> +const { makeInfallible, reportException } = DevToolsUtils;

Add safeErrorString here as it is used in a couple of other places in script.js.

@@ +918,5 @@
>      return null;
>    },
>  
>    _unknownError: function DSC__unknownError(aPrefix, aError) {
> +    let errorString = aPrefix + ": " + DevToolsUtils.safeErrorString(aError);

...and then this can remain as it was.
Attachment #8367843 - Flags: review?(past)
Forgot to note that we should also test/fix metro/,  webapprt/ and ping :Fallen about Thunderbird.
Comment on attachment 8367843 [details] [diff] [review]
Refactor DevToolsUtils.js

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

::: toolkit/devtools/server/main.js
@@ +9,5 @@
>   * Toolkit glue for the remote debugging protocol, loaded into the
>   * debugging global.
>   */
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> +const { makeInfallible, reportException } = DevToolsUtils;

The problem with b2g turned out to be that you didn't assign DevToolsUtils, makeInfallible, etc. to |this|. See a few lines below how we do the same thing for Ci, etc.
I still get this error, which I haven't tracked down though:

DBG-SERVER: error occurred while processing 'prettyPrint: ReferenceError: DevToolsUtils is not defined
Stack: SourceActor.prototype.onPrettyPrint/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:2512
reject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:133
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:52
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
reject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:134
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:52
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
reject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:134
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:52
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
reject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:134
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:52
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/promise.js:127
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:185
SourceActor.prototype._sendToPrettyPrintWorker/</onReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:2543
I can no longer reproduce the error from the previous comment, after the fixes I mentioned in comment 14. Maybe I was looking at older logs before, I don't know.
Comment on attachment 8367039 [details] [diff] [review]
Make Services requirable

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

I didn't spot anything obviously wrong in this patch, but I'm clearing the review request for now as discussed, until you finish the other patch and are ready to test them both combined.
Attachment #8367039 - Flags: review?(past)
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
I tried this patch on friday and all mochitests are green now. Marionette and B2G tests are still red however.

I made some minor changes to the patch before I uploaded it, so I'm going to check if mochitests are still green. Panos, if you could test for B2G in the meantime and tell me what to fix, I'd greatly appreciate it.
Attachment #8367843 - Attachment is obsolete: true
Attachment #8369366 - Flags: review?(past)
Comment on attachment 8369366 [details] [diff] [review]
Refactor DevToolsUtils.js

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

So the problems I found on b2g were mainly the missing qualification of makeInfallible in b2g/chrome/content/dbg-browser-actors.js. There was another redeclaration error that I mention below, but after those foxes I could get a toolbox to properly connect and debug apps. I still had some issues with the App Manager, but I'm not sure if they are related to this patch or not, so I'll have to look again.

Oh, and I also got an error on b2g from marionette, about an undefined |require|. I think this is because you need to update testing/marionette/marionette-server.js to change the way DevToolsUtils.js is loaded. Maybe this will fix the marionette tests, too.

::: toolkit/devtools/server/child.js
@@ +3,1 @@
>  const {DebuggerServer, ActorPool} = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});

I got a redeclaration error for devtools here on b2g, so I would suggest that you just use |let| instead of |const|. I believe there is another bug on file to fix things when the frame script is called multiple times.

::: toolkit/devtools/server/dbg-server.jsm
@@ +22,5 @@
> +/**
> + * To support chrome debugging, all debugger server globals should be invisible
> + * to the debugger. To facilitate this, we give the debugger server its own
> + * custom loader, with invisibleToDebugger set to true.
> + */

This comment shouldn't be here either.
Attachment #8369366 - Flags: review?(past)
(In reply to Panos Astithas [:past] from comment #19)
> could get a toolbox to properly connect and debug apps. I still had some
> issues with the App Manager, but I'm not sure if they are related to this
> patch or not, so I'll have to look again.

Turns out that those issues are bug 966988, so we're good here.
Fennec seems to work fine with an updated patch per comment 19. Metro should be fine too from what I can tell reading the metro-specific code, but you should ask mbrubeck for feedback on the updated patch. You should be able to build and test webapprt yourself, but I don't expect any issues there either.
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
With your suggestions, I now managed to get the marionette tests to work as well. The only thing that doesn't work yet are the B2G tests. Since you managed to get them to work, I'm likely overlooking something simple, so I appreciate it if you could take another look at it:

Relevant try run:
https://tbpl.mozilla.org/?tree=Try&rev=86c70716ee8d
Attachment #8369366 - Attachment is obsolete: true
Attachment #8371429 - Flags: review?(past)
Attachment #8371429 - Attachment description: patch → Refactor DevToolsUtils.js
Comment on attachment 8371429 [details] [diff] [review]
Refactor DevToolsUtils.js

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

::: testing/marionette/marionette-server.js
@@ +39,5 @@
>  this.dumpn = function dumpn(str) {
>    logger.trace(str);
>  }
> +let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils.js");

I see the following in all the logs I looked at, which makes me suspect that you need to stick DevToolsUtils to |this| here:

Marionette	INFO	marionette-server.js loaded
Marionette	ERROR	exception: ReferenceError, DevToolsUtils is not defined
Attachment #8371429 - Flags: review?(past)
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
The latest try run looks good enough for this patch to be reviewed for landing:
https://tbpl.mozilla.org/?tree=Try&rev=b99de9400cdb
Attachment #8371429 - Attachment is obsolete: true
Attachment #8381688 - Flags: review?(past)
Comment on attachment 8381688 [details] [diff] [review]
Refactor DevToolsUtils.js

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

Looks good to me. Let's get an official review for the marionette change as well.

::: toolkit/devtools/DevToolsUtils.jsm
@@ +9,4 @@
>   *
>   * To support chrome debugging, the debugger server needs to have all its
>   * code in one global, so it must use loadSubScript directly. Everyone else,
>   * though, prefers a JSM.

This comment is no longer accurate, better change it to note that the debugger server uses the devtools loader.

::: toolkit/devtools/server/actors/tracer.js
@@ +140,2 @@
>                        new Error("Ignoring request to add the debugger's "
>                                  + "compartment as a debuggee"));

Style nit: the arguments are no longer properly aligned. Might want to use a local variable for the message to cut down on indentation.

@@ +609,1 @@
>                        new Error("Failed to provide a grip for: " + aValue));

Same here.
Attachment #8381688 - Flags: review?(past)
Attachment #8381688 - Flags: review?(jgriffin)
Attachment #8381688 - Flags: review+
Status: NEW → ASSIGNED
Comment on attachment 8381688 [details] [diff] [review]
Refactor DevToolsUtils.js

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

r+ for the Marionette part
Attachment #8381688 - Flags: review?(jgriffin) → review+
Note that bug 976679 also makes the same modification to Loader.jsm, so whoever lands last will have to rebase.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3802bdd23b74
Whiteboard: [feature] p=0 → [leave-open] [feature] p=0
This patch makes the Services object a built-in module of the devtools loader. This allows us to isolate all uses of Cu.import for this object into Loader.jsm.

I have a green try run for this patch, so it's ready to land once it gets r+'d:
https://tbpl.mozilla.org/?tree=Try&rev=c10d9f52872e
Attachment #8367039 - Attachment is obsolete: true
Attachment #8388872 - Flags: review?(past)
Attached patch Refactor script.js (obsolete) — Splinter Review
This patch refactors script.js to be require-friendly. To accomplish this, I've moved some of the debug functions like dbg_assert and dumpn to a separate file, and made the Debugger object a predefined module of the devtools loader.

Note that there are still some Cu.import's left in script.js. I intend to get rid of those in a followup patch.

The try run for this patch was still running when I posted this, so there might still be some failing tests, but locally it looked promising:
https://tbpl.mozilla.org/?tree=Try&rev=ec76e1ec1c54
Attachment #8388878 - Flags: review?(past)
Panos, the try run in comment #31 shows B2G test failures. Can you help me figure those out?
Flags: needinfo?(past)
(In reply to Eddy Bruel [:ejpbruel] from comment #32)
> Panos, the try run in comment #31 shows B2G test failures. Can you help me
> figure those out?

I'll be attending a git meetup today so my availability will be limited, but I'd still like to try and work together to get this patch green, if you can find the time today to help me out :)
The b2g failures are caused by this:

Marionette	ERROR	exception: NS_ERROR_ILLEGAL_VALUE, Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [IJSDebugger.addClass]

Loading jsdebugger.jsm inside the loader is the obvious culprit. My guess is that the error is coming from this code:

http://dxr.mozilla.org/mozilla-central/source/js/ductwork/debugger/JSDebugger.cpp#56
Flags: needinfo?(past)
(In reply to Panos Astithas [:past] from comment #34)
> The b2g failures are caused by this:
> 
> Marionette	ERROR	exception: NS_ERROR_ILLEGAL_VALUE, Component returned
> failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [IJSDebugger.addClass]
> 
> Loading jsdebugger.jsm inside the loader is the obvious culprit. My guess is
> that the error is coming from this code:
> 
> http://dxr.mozilla.org/mozilla-central/source/js/ductwork/debugger/
> JSDebugger.cpp#56

Wait, why is that a problem?

Why would the global of the unwrapped global be different from the global here?
Just tested on a device and the same error is thrown from the devtools server, so it's not just a marionette issue.
Comment on attachment 8388872 [details] [diff] [review]
Refactor Services.jsm

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

::: toolkit/devtools/Loader.jsm
@@ +48,5 @@
>  BuiltinProvider.prototype = {
>    load: function() {
>      this.loader = new loader.Loader({
>        modules: {
> +        "Services": Object.create(Services),

Genius!

::: toolkit/devtools/server/protocol.js
@@ -12,5 @@
>  let events = require("sdk/event/core");
>  let object = require("sdk/util/object");
>  
> -// For telemetry
> -Cu.import("resource://gre/modules/Services.jsm");

Could you carry that comment along?
Attachment #8388872 - Flags: review?(past) → review+
Attached patch Refactor script.js (obsolete) — Splinter Review
With Panos' help, I managed to get the B2G tests to pass, but now some of the browser mochitests that were green before turned orange. It's not obvious to me what's going on, and I can't reproduce the test failures locally.
Attachment #8388878 - Attachment is obsolete: true
Attachment #8388878 - Flags: review?(past)
Attachment #8390415 - Flags: review?(past)
Comment on attachment 8390415 [details] [diff] [review]
Refactor script.js

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

I haven't figured out why the tests fail, but I have a few minor comments.

::: toolkit/devtools/Loader.jsm
@@ +12,5 @@
> +
> +// The Debugger object can only be defined on a global. Because B2G uses shared
> +// globals, the scope object in which Loader.jsm is evaluated is not guaranteed
> +// to be a global. To work around this, we create a Sandbox, and define the
> +// Debugger object on the Sandbox's global.

I'm curious to know what was the issue with just passing the actual global to addDebuggerToGlobal(). It's not that this is any harder to get a reference to the Debugger constructor for chrome code.

@@ +19,5 @@
> +    "Components.utils.import('resource://gre/modules/jsdebugger.jsm');",
> +    "addDebuggerToGlobal(this);"
> +  ].join("\n"),
> +  sandbox
> +)

Nit: missing semicolon.
Nit^2: wouldn't it be slightly faster to concatenate strings and skip [].join()?

let str = "Components.utils.import('resource://gre/modules/jsdebugger.jsm');" + "addDebuggerToGlobal(this);";
Cu.evalInSandbox(str, sandbox);

::: toolkit/devtools/server/main.js
@@ +37,5 @@
>  
>  const DBG_STRINGS_URI = "chrome://global/locale/devtools/debugger.properties";
>  
>  const nsFile = CC("@mozilla.org/file/local;1", "nsIFile", "initWithPath");
> +// Cu.import("resource://gre/modules/reflect.jsm");

You forgot to remove this.

@@ +378,5 @@
>    /**
>     * Install tab actors.
>     */
>    addTabActors: function() {
> +    require("devtools/server/actors/script.js");

Nit: drop the .js extension.

::: toolkit/devtools/server/transport.js
@@ +12,3 @@
>  Components.utils.import("resource://gre/modules/NetUtil.jsm");
>  
>  let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");

You can now get this from debug.js above.
Attachment #8390415 - Flags: review?(past)
No longer blocks: metrobacklog
Whiteboard: [leave-open] [feature] p=0 → [leave-open]
Attached patch Refactor script.js (obsolete) — Splinter Review
Here is the latest version of my script.js refactor. Unfortunately, I'm still seeing failing tests on try that I can't reproduce locally.

Nick suspects there may be a race going on, because the log output shows the correct messages are being sent.

Nick, if you could take a look at this for me and figure out what's going on, I'd greatly appreciate it!
Attachment #8390415 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)
I can repro locally:

 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-03.js | Should get 6 frames. - Got 3, expected 6
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-03.js | And none of them are black boxed. - Got 1, expected 0
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-03.js | The source should be black boxed now.
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-03.js | Should only get 3 frames. - Got 6, expected 3
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-03.js | And one of them should be the combined black boxed frames. - Got 0, expected 1
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-05.js | The first item in the deck should be selected (the source editor). - Got 1, expected 0
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-05.js | The second item in the deck should be selected (the black box message). - Got 0, expected 1
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_cmd-blackbox.js | blackbox_one should not be black boxed because it doesn't match the glob.
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-02.js | The source should be pretty printed.
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-04.js | The bar function's non-pretty-printed location should be shown.
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-04.js | The bar function's pretty printed location should be shown.
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-11.js | Test timed out
 4:31.10 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-11.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/debugger/test/doc_pretty-print.html
Flags: needinfo?(nfitzgerald)
...aaaaand of course those failing tests only fail when you run the whole suite :(
Ok.

So I've been investigating browser_dbg_blackboxing-03.js. For whatever reason, it seems that the black boxed sources from previous tests are persisting to this test. Perhaps your patch introduces this behavior somehow? It definitely didn't to that before. This data is stored here: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4717

I suspect that this is the same issue with the prettified sources, since they are stored the same way.

Eddy, looks like you have two choices moving forward:

1) Rewrite the tests to undo the black boxing and pretty printing at the end of each test.

2) Figure out why your patch introduces this saving of state that didn't exist before and make it stop.
Attached patch Refactor script.js (obsolete) — Splinter Review
With nick's feedback and a lot of help from Victor I finally managed to get a green try run for this patch!

https://tbpl.mozilla.org/?tree=Try&rev=691d58dbaab8
Attachment #8395971 - Attachment is obsolete: true
Attachment #8399386 - Flags: review?(past)
Eddy, Just a quick note to let you know about bug 988237, that will most likely merge conflict with your patch. Also in this bug I'm facing the inter-dependencies between main.js, script.js, webbrowser.js and webconsole.js. So we may fix stuff in each of our patches.
Comment on attachment 8399386 [details] [diff] [review]
Refactor script.js

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

::: toolkit/devtools/server/main.js
@@ +285,5 @@
> +    // the server is shut down. Eventually, I'd like to split all global state
> +    // off into a separate object with a properly managed lifetime.
> +    ThreadActor.breakpointStore = new BreakpointStore();
> +    ThreadSources._blackBoxedSources.clear();
> +    ThreadSources._prettyPrintedSources.clear();

It looks like you might want a dedicated loader for DebuggerServer.
That would allow you to load brand new modules,
and also ensure trashing all the previous server
(thanks to the nuke sandbox feature).

Spawning a new loader should be as easy as:
  const { DevtoolsLoader } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
  let loader = DevtoolsLoader();
  let server = loader.main("devtools/server/main");
  ...
  loader.unload();


But at the end the issue isn't all about modules, it is about having global state on the classes instead of having all the state being set only on instances objects.
Comment on attachment 8399386 [details] [diff] [review]
Refactor script.js

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

There are a few comments and oversights, but I'm very happy you got the tests passing now!

::: toolkit/devtools/Loader.jsm
@@ +12,5 @@
> +
> +// The Debugger object can only be added to a global. However, on B2G,
> +// compartment sharing causes Loader.jsm to be evaluated with a this object that
> +// is not a global. To work around this, we add the Debugger object to a sandbox
> +// instead of the this object.

You didn't answer my question from comment 39, why do we need the sandbox? It's not something I'm particularly against, but I can't see any substantial reason for it.

::: toolkit/devtools/server/actors/script.js
@@ +7,5 @@
>  
> +const { Cc, Ci, Cu, components } = require("chrome");
> +const Debugger = require("Debugger");
> +const Services = require("Services");
> +const { dbg_assert, dumpn, wantLogging } = require("devtools/server/debug");

wantLogging is no longer exported separately.

@@ +10,5 @@
> +const Services = require("Services");
> +const { dbg_assert, dumpn, wantLogging } = require("devtools/server/debug");
> +const { ActorPool, DebuggerServer } = require("devtools/server/main");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +const promise = require("sdk/core/promise");

The devtools loader now exposes a promise global, so this isn't needed. It just reverts to the sdk promises, just as we managed to switch to Promise.jsm.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +let devtools_ = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;

Nit: I'm now used to this C++ local reference style, but I expect others would find something like _devtools more natural in JS.

::: toolkit/devtools/server/debug.js
@@ +27,5 @@
> +
> +exports.dbg_assert = dbg_assert;
> +
> +function dumpn(str) {
> +  if (exports.wantLogging) {

This should be dumpn.wantLogging.

@@ +37,5 @@
> +// exports object is frozen by the loader, we define it on dumpn instead.
> +dumpn.wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> +
> +Services.prefs.addObserver("devtools.debugger.log", function () {
> +  dump.wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");

This should assign to dumpn.wantLogging, note the missing "n".

Where are we removing this observer? We can't just leak it.

::: toolkit/devtools/server/main.js
@@ +270,5 @@
> +    // defined in this file. To avoid a circular dependency, we have to
> +    // require script lazily, after main.js has already been loaded.
> +    let { BreakpointStore, ThreadActor, ThreadSources } = require("devtools/server/actors/script");
> +
> +    // This is monumentally stupid. We define the properties here below as class

I don't agree with this characterization. The tradeoffs in the design that used mozIJSSubscriptLoader were different than the current ones. What used to make sense back then doesn't necessarily make sense now and vice versa.

Keep in mind that besides separate thread actors for different tabs, a DebuggerServer instance would create separate thread actors for different (simultaneous) client connections to the same tab. In that case we would want these stores to be shared. The alternative would be to put that state in instance objects on the DebuggerServerConnection, which would be less elegant than what we have now.

Can we please drop that comment?

@@ +285,5 @@
> +    // the server is shut down. Eventually, I'd like to split all global state
> +    // off into a separate object with a properly managed lifetime.
> +    ThreadActor.breakpointStore = new BreakpointStore();
> +    ThreadSources._blackBoxedSources.clear();
> +    ThreadSources._prettyPrintedSources.clear();

When script.js is converted to use protocol.js, this will no longer be necessary here, as this cleanup could then take place in the module's unregister() call.

@@ +368,5 @@
> +      // script.js has a top-level dependency on DebuggerServer, which is
> +      // defined in this file. To avoid a circular dependency, we have to
> +      // require script lazily, after main.js has already been loaded.
> +      let { ChromeDebuggerActor } = require("devtools/server/actors/script");
> +      this.addGlobalActor(ChromeDebuggerActor, "chromeDebugger");

I'm not sure I understand the necessity of this. Since addBrowserActors is called from the addTabActors() call above, why not just stick ChromeDebuggerActor to |this| there, instead of requiring it again here?

I'm also confused by the comment. Since we were loading the script actors inside addTabActors(), it was already loaded lazily, right?

::: toolkit/devtools/server/transport.js
@@ +191,5 @@
>        dump(msg + "\n");
>        return true;
>      }
>  
> +    dumpn("Got: " + JSON.stringify(parsed, null, 2));

Why did you remove the wantLogging check?
Attachment #8399386 - Flags: review?(past)
Attached patch Failing marionette tests (obsolete) — Splinter Review
I've broken the script.js up in several smaller patches in an attempt to make this easier to land, but it looks like I messed up the marionette server again as a result:

https://tbpl.mozilla.org/?tree=Try&rev=398804f97c4c

I've attached the diff for all the patches combined. I'm pretty sure this is something simple, so if you could take a look at the logs for me, I'd appreciate it.
Attachment #8399386 - Attachment is obsolete: true
Attachment #8401259 - Flags: feedback?(past)
Comment on attachment 8401259 [details] [diff] [review]
Failing marionette tests

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

::: toolkit/devtools/server/transport.js
@@ +13,5 @@
> +  Cu.import("resource://gre/modules/NetUtil.jsm");
> +}
> +
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +const { dumpn } = require("devtools/server/debug");

I see this, presumably because of the above:

exception: TypeError, redeclaration of var DevToolsUtils

You probably want to remove the DevToolsUtils include in marionette-server.js.
Attachment #8401259 - Flags: feedback?(past)
The Debugger constructor will be provided differently depending on whether we're on the main thread or in a worker. Since the worker debugger will use its own loader, I propose providing the Debugger constructor as a global via the loader to hide this fact.

In general, I want to propose exposing those parts of the API expected by the debugger server that are shared between the main thread and workers as globals on the loader.
Attachment #8401259 - Attachment is obsolete: true
Attachment #8401745 - Flags: review?(past)
This patch factors out common debug utilities used by the server into a common file.

The point of this refactor is that we don't want to have each actor cyclically depend on main.js, where these functions are defined at the moment, as we refactor them to be required in their own separate global.
Attachment #8401746 - Flags: review?(past)
Similar to the previous patch
Attachment #8401747 - Flags: review?(past)
Attached patch Factor out ActorPool (obsolete) — Splinter Review
Attachment #8401748 - Flags: review?(past)
Attachment #8401749 - Flags: review?(past)
Attachment #8401749 - Attachment description: patch → Refactor transport.js to be require friendly
This is what's left of the original script.js patch, after I factored some things out into separate patches. I addressed all your review comments afaict, but I may have overlooked some things. Apologies if that's the case.

Note that this patch and the next are still pending on a green try run.
Attachment #8401751 - Flags: review?(past)
Attached patch Refactor root.js (obsolete) — Splinter Review
Attachment #8401752 - Flags: review?(past)
I'm seeing a test failure on B2G for these patches that I can't trace back to anything I changed. More bizarrely, I'm seeing a crash in graphics code on Windows that I cannot explain.

I'm currently bisecting on try to figure out what patch introduces the issue. In the meantime, could you take a look at the B2G logs for me? I've attached the combined patch.
Attachment #8401791 - Flags: feedback?(past)
Looks like the culprit for the windows crash is the DevToolsUtils.js refactor. At least that narrows the scope of the problem quite a bit. The actual cause is still a mystery to me, however.

https://tbpl.mozilla.org/?tree=Try&rev=3aa7d5d8d8bd
Comment on attachment 8401745 [details] [diff] [review]
Provide Debugger constructor as a global

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

Looks good.

::: toolkit/devtools/server/main.js
@@ -38,5 @@
>  Cu.import("resource://gre/modules/reflect.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  let wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
>  
> -Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

Some toe-stepping here. I think it would be easiest to just let bug 990400 clean up promises.
Attachment #8401745 - Flags: review?(past) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #61)
> Looks like the culprit for the windows crash is the DevToolsUtils.js
> refactor. At least that narrows the scope of the problem quite a bit. The
> actual cause is still a mystery to me, however.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=3aa7d5d8d8bd

As I mentioned on IRC, the Windows assertion seems to be bug 963492. This patch must be doing something that exacerbates that problem.
Comment on attachment 8401746 [details] [diff] [review]
Factor out common debug functions

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

::: toolkit/devtools/server/debug.js
@@ +14,5 @@
> +  var Services = require("Services");
> +}
> +
> +function observer() {
> +  dump.wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");

s/dump/dumpn/

@@ +17,5 @@
> +function observer() {
> +  dump.wantLogging = Services.prefs.getBoolPref("devtools.debugger.log");
> +};
> +
> +function deinit() {

I think a name like "cleanup" would be more descriptive.

@@ +35,5 @@
> +    }
> +  }
> +};
> +
> +// We want shouldThrow to be writable so we can set it to true in head_dbg.js.

I don't see the changes to head.js and head_dbg.js in this patch.

::: toolkit/devtools/server/main.js
@@ +274,5 @@
>  
>      this._fireConnectionChange("closed");
>  
>      dumpn("Debugger server is shut down.");
> +    deinit();

This should go before the dumpn() call.
Attachment #8401746 - Flags: review?(past) → review-
Comment on attachment 8401747 [details] [diff] [review]
Factor out common actor functions

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

::: toolkit/devtools/server/actors/root.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +let devtools_ = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +let { createExtraActors, appendExtraActors } = devtools_.require("devtools/server/common");

"common" is too generic a name, how about something like "actor-creation-helpers"? Dashes instead of camel-case to follow the loader convention.

::: toolkit/devtools/server/common.js
@@ +15,5 @@
> + * actors are already there. Add all actors in the final extra actors table to
> + * |aPool|.
> + *
> + * The root actor and the tab actor use this to instantiate actors that other
> + * parts of the browser have specified with DebuggerServer.addTabActor antd

Typo: s/antd/and/
Attachment #8401747 - Flags: review?(past) → review+
Comment on attachment 8401746 [details] [diff] [review]
Factor out common debug functions

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

::: toolkit/devtools/server/debug.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// Common helper functions used for debugging.

Also, why not put all of this into DevToolsUtils.js to cut down on file proliferation? It isn't server-only either.
Comment on attachment 8401748 [details] [diff] [review]
Factor out ActorPool

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

This looks fine technically, but you didn't explain why we need to separate the ActorPool code in a different module. It may make the server more modular, but that comes at a non-zero memory cost (see the recent "too many compartments" thread in dev-platform or our 128 MB device discussion the other day). Are we getting something substantial out of this?
Attachment #8401748 - Flags: review?(past)
Comment on attachment 8401749 [details] [diff] [review]
Refactor transport.js to be require friendly

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

::: testing/marionette/marionette-server.js
@@ +41,5 @@
>  }
>  let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>  let DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils.js");
>  this.DevToolsUtils = DevToolsUtils;
> +let { DebuggerTransport } = devtools.require("devtools/server/transport");

Don't you have to stick DebuggerTransport to |this|? Could this be why gaia tests are failing?

::: toolkit/devtools/Loader.jsm
@@ +43,5 @@
>   * Providers are different strategies for loading the devtools.
>   */
>  
> +function setTimeout(callback, delay) {
> +  Services.tm.currentThread.dispatch(callback, delay);

This is not actually a delay, it's a bit flag:

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIEventTarget.idl#30

And that is why setTimeout is not a good name for it. I'm kind of torn between setImmediate and executeSoon. Probably the latter, to avoid more confusion (from what I've read setImmediate might actually do the opposite thing of executeSoon).

I would even suggest to just have a single callback argument and always pass Ci.nsIThread.DISPATCH_NORMAL, as I don't believe we have any need to dispatch a runnable synchronously.

@@ +59,5 @@
>      lazyGetter: XPCOMUtils.defineLazyGetter.bind(XPCOMUtils),
>      lazyImporter: XPCOMUtils.defineLazyModuleGetter.bind(XPCOMUtils),
>      lazyServiceGetter: XPCOMUtils.defineLazyServiceGetter.bind(XPCOMUtils)
> +  },
> +  setTimeout: setTimeout

That object literal could use some sorting, but oh well.

::: toolkit/devtools/server/main.js
@@ +10,5 @@
>   * debugging global.
>   */
>  let Services = require("Services");
>  let { ActorPool } = require("devtools/server/ActorPool");
> +let { ChildDebuggertransport, DebuggerTransport, LocalDebuggerTransport } = require("devtools/server/transport");

Typo in ChildDebuggerTransport. Another potential reason for the failed gaia tests.
Attachment #8401749 - Flags: review?(past) → review-
(In reply to Panos Astithas [:past] from comment #67)
> Comment on attachment 8401748 [details] [diff] [review]
>
> It may make the server more
> modular, but that comes at a non-zero memory cost (see the recent "too many
> compartments" thread in dev-platform or our 128 MB device discussion the
> other day). 

About that, that would be really handy if we can have a similar tweak for SDK modules like the one we have for JSMs on b2g! (One global for all JSMs, each of them being evaluated in a [may be special] closure). The thing is that Compartment Per Global and zones aren't as effecient as we think (see bug 989373). So having one global, so one compartment, per module can end up being very expensive.
Comment on attachment 8401751 [details] [diff] [review]
Refactor script.js to be require friendly

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

::: toolkit/devtools/server/actors/script.js
@@ +9,5 @@
> +  var { Cc, Ci, Cu, components } = require("chrome");
> +  var Services = require("Services");
> +
> +  Cu.import("resource://gre/modules/NetUtil.jsm");
> +  Cu.import("resource://gre/modules/devtools/SourceMap.jsm");

Might not be important enough for a v1, but source-mapped worker sources will need this. The reason I mention it is that I think that switching from Cu.import("SourceMap.jsm") to require("source-map") might be all that is needed for this.

Do a quick test: replace Cu.import with the require() call above and see if xpcshell tests pass locally. If not, we will have to get Nick's feedback.

@@ +15,5 @@
> +
> +const { dbg_assert, dumpn, wantLogging } = require("devtools/server/debug");
> +const { ActorPool, DebuggerServer } = require("devtools/server/main");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +const { all, defer, resolve } = require("sdk/core/promise");

Why is this needed when the loader provides a global promise identifier?

@@ +25,5 @@
> +exports.unregister = function (handle) {
> +  ThreadActor.breakpointStore = new BreakpointStore();
> +  ThreadSources._blackBoxedSources.clear();
> +  ThreadSources._prettyPrintedSources.clear();
> +};

Unregistering the module should also remove the global actor set in register(). Furthermore, the global state that is cleared here, should also be initialized in register() instead of the module's global scope. This would avoid re-entrancy issues, like bug 986841.

::: toolkit/devtools/server/main.js
@@ -375,5 @@
>    /**
>     * Install tab actors.
>     */
>    addTabActors: function() {
> -    this.addActors("resource://gre/modules/devtools/server/actors/script.js");

How are the script actors going to be loaded in tabs now? On B2G at least (and probably e10s too) we need to load them here, as execution might be under restricted privileges, which would preclude the registerModule() call in addBrowserActors().
Attachment #8401751 - Flags: review?(past) → review-
Comment on attachment 8401752 [details] [diff] [review]
Refactor root.js

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

I don't have a lot of questions on this, but since it depends on others that need changes, I'd like to see it again.

::: toolkit/devtools/server/actors/script.js
@@ +2176,5 @@
>     * @param aString String
>     *        The string we are creating a grip for.
>     */
>    threadLongStringGrip: function (aString) {
> +    return this.longStringGrip(aString, this.threadLifetimePool);

Out of curiosity, why was this necessary?

::: toolkit/devtools/server/actors/webconsole.js
@@ +13,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +let devtools_ = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +let { ActorPool } = devtools_.require("devtools/server/ActorPool");
> +let { EnvironmentActor, LongStringActor, ObjectActor, ThreadActor } = devtools_.require("devtools/server/actors/script");

Why the devtools->devtools_ rename? Does it clash with a loader-provided name? If so, why not just use that instead?

::: toolkit/devtools/server/main.js
@@ -216,5 @@
>      }
>  
>      this.xpcInspector = Cc["@mozilla.org/jsinspector;1"].getService(Ci.nsIJSInspector);
>      this.initTransport(aAllowConnectionCallback);
> -    this.addActors("resource://gre/modules/devtools/server/actors/root.js");

I'm starting to get confused by the module dependencies with all these separate patches. I'll take a closer look at the combined patch after your next round of updates.
Attachment #8401752 - Flags: review?(past)
Comment on attachment 8401791 [details] [diff] [review]
Failing B2g tests and crashing on Windows

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

I think the failures must be caused by one of the bugs I've mentioned in previous comments.
Attachment #8401791 - Flags: feedback?(past)
Attached patch Refactor DevToolsUtils.js (obsolete) — Splinter Review
This is all green now, but requires a r+ before it can land. Panos, can you take a quick look at this?
Attachment #8381688 - Attachment is obsolete: true
Attachment #8404632 - Flags: review?(past)
Comment on attachment 8404632 [details] [diff] [review]
Refactor DevToolsUtils.js

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

::: toolkit/devtools/DevToolsUtils.js
@@ +8,3 @@
>  
> +// hasChrome is provided as a global by the loader. It is true if we are running
> +// on the main thread, and false if we are running on a worker thread.

I think this comment would be useful in the loader as well.

@@ +10,5 @@
> +// on the main thread, and false if we are running on a worker thread.
> +if (hasChrome) {
> +  var { Ci, Cu } = require("chrome");
> +  var Services = require("Services");
> +  var setTimeout = Cu.import("resource://gre/modules/Timer.jsm", {}).setTimeout;

Nit: without the destructuring assignment it just got longer :-)
Attachment #8404632 - Flags: review?(past) → review+
Comment on attachment 8404632 [details] [diff] [review]
Refactor DevToolsUtils.js

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

::: toolkit/devtools/DevToolsUtils.js
@@ +11,5 @@
> +if (hasChrome) {
> +  var { Ci, Cu } = require("chrome");
> +  var Services = require("Services");
> +  var setTimeout = Cu.import("resource://gre/modules/Timer.jsm", {}).setTimeout;
> +}

This doesn't look like a good practice.

We should either:
- set all these things as globals in both loaders,
- expose all these things through pseudo modules in both loaders (like what you did for Services).
But oh wait, is there a loader (sdk loader) in the workers?

Also note that, for setTimeout, we do have a require("sdk/timers").setTimeout in the SDK. (if you want to just remove the Cu.import usage)

Finally, I don't see what is the overall goal here. Are you aiming to drop all require/Cu.import in debugger server for worker support??
I've decided to go about it slightly differently, and provide the Debugger constructor as a built-in module, rather than a global. The patch is largely the same as the previous one that you r+'d, but I'd like you to take another look at it just in case.
Attachment #8401745 - Attachment is obsolete: true
Attachment #8407542 - Flags: review?(past)
As we discussed on irc, I decided not to use hasChrome to decide whether chrome APIs can be required. Instead, we will make them requirable on workers. However, they will return vacuous objects there, so that any attempt to use these APIs will cause an exception.

As per your review comments, I've also moved the common debug functions to DevToolsUtils.js in this patch, rather than give them their own file.

I've also gotten rid of the pref observer in the previous patch, because it led to issues where logging didn't work as I expected. Instead, I've made sure to set wantLogging at the exact same time as we did before, to make sure our behavior doesn't change.
Attachment #8401746 - Attachment is obsolete: true
Attachment #8404632 - Attachment is obsolete: true
Attachment #8407545 - Flags: review?(past)
Depends on: 997239
(In reply to Panos Astithas [:past] from comment #70)
> Comment on attachment 8401751 [details] [diff] [review]
> Refactor script.js to be require friendly
> 
> Review of attachment 8401751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +9,5 @@
> > +  var { Cc, Ci, Cu, components } = require("chrome");
> > +  var Services = require("Services");
> > +
> > +  Cu.import("resource://gre/modules/NetUtil.jsm");
> > +  Cu.import("resource://gre/modules/devtools/SourceMap.jsm");
> 
> Might not be important enough for a v1, but source-mapped worker sources
> will need this. The reason I mention it is that I think that switching from
> Cu.import("SourceMap.jsm") to require("source-map") might be all that is
> needed for this.
> 
> Do a quick test: replace Cu.import with the require() call above and see if
> xpcshell tests pass locally. If not, we will have to get Nick's feedback.
> 
> @@ +15,5 @@
> > +
> > +const { dbg_assert, dumpn, wantLogging } = require("devtools/server/debug");
> > +const { ActorPool, DebuggerServer } = require("devtools/server/main");
> > +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> > +const { all, defer, resolve } = require("sdk/core/promise");
> 
> Why is this needed when the loader provides a global promise identifier?
> 
> @@ +25,5 @@
> > +exports.unregister = function (handle) {
> > +  ThreadActor.breakpointStore = new BreakpointStore();
> > +  ThreadSources._blackBoxedSources.clear();
> > +  ThreadSources._prettyPrintedSources.clear();
> > +};
> 
> Unregistering the module should also remove the global actor set in
> register(). Furthermore, the global state that is cleared here, should also
> be initialized in register() instead of the module's global scope. This
> would avoid re-entrancy issues, like bug 986841.
> 
> ::: toolkit/devtools/server/main.js
> @@ -375,5 @@
> >    /**
> >     * Install tab actors.
> >     */
> >    addTabActors: function() {
> > -    this.addActors("resource://gre/modules/devtools/server/actors/script.js");
> 
> How are the script actors going to be loaded in tabs now? On B2G at least
> (and probably e10s too) we need to load them here, as execution might be
> under restricted privileges, which would preclude the registerModule() call
> in addBrowserActors().

Currently, the call to addActors loads the script actors in the scope of the DebuggerServer instance represented by the this object. All consumers of the script actors are similarly loaded in the same scope, so they can access and call the script actor constructors.

However, when we make script.js requirable, it will be loaded in its own, separate scope, so this trick will no longer work. The obvious solution to this is to have all consumers of the script actors explicitly require their dependencies from script.js (which is what this patch does).

This only holds for script actors created via an explicit constructor call. Other actors, such as ChromeDebuggerActor, are created implicitly via a factory function, that is registered with either addTabActor or addGlobalActor (in this case its the latter).

Currently, we register ChromeDebuggerActor in addBrowserActors. Since the call to addTabActors in that function adds ChromeDebuggerActor to this, we can then call addGlobalActor on the next line. However, if we no longer call addActors for script.js in addTabActors, ChromeDebuggerActor will no longer be defined. The obvious solution is to replace the call to addGlobalActor in addBrowserActors with a call to registerModule, which is what this patch does. 

Hopefully that clarifies things. Your other comments seem valid, so I will address them in the upcoming patch.
Oh, I just realized that that's not entirely accurate. We can't use registerModule to register ChromeDebuggerActor, becasue we also need to call it to initialize the class properties in script.js. This needs to happen even if we don't want ChromeDebuggerActor to be registered.

The obvious solution is to require ChromeDebuggerActor separately and register it in addBrowserActors, as we do now.

The part about the call to addActors no longer being necessary is still accurate, afaict.
This patch moves both the common actor functions and ActorPool into a single separate file. Since I previously moved ActorPool into its own file, I already had an r+ for the first part, but not the second.

I've got a green try run for the patches I've filed so far, so as soon as I get an r+, this can land:
https://tbpl.mozilla.org/?tree=Try&rev=135514336f4c
Attachment #8401747 - Attachment is obsolete: true
Attachment #8401748 - Attachment is obsolete: true
Attachment #8407847 - Flags: review?(past)
Attached patch Make Timer a built-in module (obsolete) — Splinter Review
This patch makes Timer.jsm a built-in module, same as Services. The idea is that the Timer module will be available in workers as well, but will be provided differently. By making it a built-in module we can abstract away the details of how it is provided in the loader.
Attachment #8407849 - Flags: review?(past)
This patch wasn't properly update. Sorry!
Attachment #8407849 - Attachment is obsolete: true
Attachment #8407849 - Flags: review?(past)
Attachment #8407858 - Flags: review?(past)
I've addressed your review comments (removed the typo, made sure to add DebuggerTransport etc. to this), and fixed a bug in the previous patch where I forgot to require transport.js in the marionette server.

Unfortunately, it looks like the marionette tests are still broken on this try push: https://tbpl.mozilla.org/?tree=Try&rev=c5ac2e0c88cb

I still don't have a B2G phone, so I can't test this locally. I've looked at the try logs, but couldn't find any useful error message. I've checked the patch several times, but can't really come up with anything else to try.

Could you help me with this tomorrow?
Attachment #8401749 - Attachment is obsolete: true
Attachment #8407863 - Flags: feedback?(past)
I've refactored script.js as per your review comments, but bizarelly, I'm running into strict errors for things that are supposed to be legal in JS afaik:

/Users/ejpbruel/Projects/fx-team/testing/xpcshell/head.js:723: strict error: reference to undefined property stack.name

How can this patch possible cause code to be run with strict errors enabled?
Attachment #8401751 - Attachment is obsolete: true
Attachment #8407912 - Flags: feedback?(past)
Attachment #8407912 - Attachment description: Refactor script.js → Refactor script.js (strict errors)
With help from fitzgen, I've figured out that the problem is that in the failing tests (I've looked at test_blackboxing-06.js as an example) we call do_check_true, defined in testing/xpcshell/head.js, with a string as its second argument. However, instead of a string it expects an optional stack. The result is that we end up in do_report_result with stack set to a string, which do_report_result is not prepared to handle.

The part that I haven't figured out yet is that we actually end up in head.js:718, which means that the test succeeded. But then how did this ever work? Obviously, the test passed before, but then the above bug should have triggered a strict error as well. What is going on?
Attachment #8407542 - Flags: review?(past) → review+
Attachment #8407545 - Flags: review?(past) → review+
Depends on: 998040
Attached patch Refactor transport.js (obsolete) — Splinter Review
I had a hard time figuring out what was causing the marionette server to hang. The logs didn't give me anything to go on, so I tried a different approach.

The root of the problem is that transport.js has multiple consumers, each of which use loadSubScript, which loads the file into the global of the caller. This has subtle consequences, such as transport.js relying on globals provided by the parent, and vice versa.

For now, the only consumer we care about is the debugger server itself, so why not simply refactor transport.js so it can be loaded with both require and loadSubScript? This won't affect the consumers we don't care about (for now), while still allowing the debugger server to use require (which is our immediate goal).

The resulting patch is quite simple, and I managed to get all the tests green with it:

https://tbpl.mozilla.org/?tree=Try&rev=db88d451ea78

Please let me know if you object to this approach. If you don't, I can move forward. If not, I'll contact ateam to figure out what's causing the marionette failure. However, I would like to point out that I feel that that would be a distraction from my immediate goal, which is to get the debugger server to run in a worker, not to requirify the entire server codebase.
Attachment #8407863 - Attachment is obsolete: true
Attachment #8407863 - Flags: feedback?(past)
Attachment #8408930 - Flags: feedback?(past)
Comment on attachment 8408930 [details] [diff] [review]
Refactor transport.js

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

I would have preferred the cleaner refactoring to be honest, but I can live with this.
Attachment #8408930 - Flags: feedback?(past) → feedback+
Comment on attachment 8407912 [details] [diff] [review]
Refactor script.js (strict errors)

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

If memory serves, we discussed that fixing the tests to not use the extra string argument was our best option here.
Attachment #8407912 - Flags: feedback?(past) → feedback+
Attachment #8407858 - Flags: review?(past) → review+
Attachment #8407847 - Flags: review?(past) → review+
Attachment #8407912 - Attachment is obsolete: true
Attachment #8408930 - Attachment is obsolete: true
Attachment #8410613 - Flags: review?(past)
Comment on attachment 8410613 [details] [diff] [review]
Refactor transport.js

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

::: toolkit/devtools/server/transport.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +(function (factory) { // Module boilerplate

Please file a followup bug to remove this boilerplate code once marionette is properly converted to load transport.js as an SDK module, and add a TODO comment here to that effect.
Attachment #8410613 - Flags: review?(past) → review+
Still pending a green try run, but it looks like I managed to get all the tests to pass locally.

Note that this patch depends on the patch in bug 999854. Without that,  browser_dbg_pretty-print-on-paused.js will fail.
Attachment #8401791 - Attachment is obsolete: true
Attachment #8411748 - Flags: review?(past)
Attached patch Refactor root.jsSplinter Review
Attachment #8401752 - Attachment is obsolete: true
Attachment #8411753 - Flags: review?(past)
Green try run for the script.js refactor:
https://tbpl.mozilla.org/?tree=Try&rev=72c73e140f34
(In reply to Eddy Bruel [:ejpbruel] from comment #103)
> Green try run for the script.js refactor:
> https://tbpl.mozilla.org/?tree=Try&rev=72c73e140f34

What about the Gi failure?

16:58:04     INFO -  TEST-START | Dev Tools server is running and listening
16:58:09     INFO -  TEST-UNEXPECTED-FAIL | Dev Tools server is running and listening
16:58:09     INFO -  TEST-END | Dev Tools server is running and listening

16:58:10     INFO -    JavaScriptError: (17) ReferenceError: DebuggerServer is not defined
16:58:10     INFO -    Remote Stack:
16:58:10     INFO -    execute_script @undefined, line undefined
16:58:10     INFO -    inline javascript, line 1
16:58:10     INFO -    src: "      return DebuggerServer.initialized;"
16:58:10     INFO -        at Error.MarionetteError (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)
16:58:10     INFO -        at Object.Client._handleCallback (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:477:19)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:511:21
16:58:10     INFO -        at TcpSync.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:100:10)
16:58:10     INFO -        at Object.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:458:36)
16:58:10     INFO -        at Object.Client._sendCommand (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:504:19)
16:58:10     INFO -        at Object._executeScript (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1454:19)
16:58:10     INFO -        at Object.executeScript (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1194:19)
16:58:10     INFO -        at Context.<anonymous> (/builds/slave/test/gaia/tests/integration/devtools/server_test.js:25:39)
16:58:10     INFO -        at Test.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:211:32)
16:58:10     INFO -        at Runner.runTest (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:372:10)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:448:12
16:58:10     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:297:14)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:307:7
16:58:10     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:245:23)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:269:7
16:58:10     INFO -        at Hook.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:213:5)
16:58:10     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:257:10)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:269:7
16:58:10     INFO -        at done (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:185:5)
16:58:10     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:197:9
16:58:10     INFO -        at Object.executeHook (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)
16:58:10     INFO -        at process._tickCallback (node.js:415:13)
(In reply to Panos Astithas [:past] from comment #104)
> (In reply to Eddy Bruel [:ejpbruel] from comment #103)
> > Green try run for the script.js refactor:
> > https://tbpl.mozilla.org/?tree=Try&rev=72c73e140f34
> 
> What about the Gi failure?

Oh, gah, don't know how I missed that one :(
(In reply to Panos Astithas [:past] from comment #104)
> (In reply to Eddy Bruel [:ejpbruel] from comment #103)
> > Green try run for the script.js refactor:
> > https://tbpl.mozilla.org/?tree=Try&rev=72c73e140f34
> 
> What about the Gi failure?
> 
> 16:58:04     INFO -  TEST-START | Dev Tools server is running and listening
> 16:58:09     INFO -  TEST-UNEXPECTED-FAIL | Dev Tools server is running and
> listening
> 16:58:09     INFO -  TEST-END | Dev Tools server is running and listening
> 
> 16:58:10     INFO -    JavaScriptError: (17) ReferenceError: DebuggerServer
> is not defined
> 16:58:10     INFO -    Remote Stack:
> 16:58:10     INFO -    execute_script @undefined, line undefined
> 16:58:10     INFO -    inline javascript, line 1

So how do I debug this? There is no stack. It's not even clear which file triggered the error.
Loks like it's not my patch that's responsible for the Gi bustage:

https://bugzilla.mozilla.org/show_bug.cgi?id=942756#c214

I'll make another try push to make sure.
Comment on attachment 8411748 [details] [diff] [review]
Refactor script.js

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

r=me with the missing addTabActor() call added.

::: toolkit/devtools/server/actors/script.js
@@ +5627,5 @@
> +exports.register = function(handle) {
> +  ThreadActor.breakpointStore = new BreakpointStore();
> +  ThreadSources._blackBoxedSources = new Set(["self-hosted"]);
> +  ThreadSources._prettyPrintedSources = new Map();
> +};

registerModule() expects the actor module to call addTabActor/addGlobalActor itself. There is an addGlobalActor call in addBrowserActors(), but there is no corresponding call for addTabActor() anywhere now. You should add one here, along with a corresponding removeTabActor() call in unregister() below. See tracer.js for an example.
Attachment #8411748 - Flags: review?(past) → review+
Comment on attachment 8411753 [details] [diff] [review]
Refactor root.js

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

Looks good to me.
Attachment #8411753 - Flags: review?(past) → review+
No longer blocks: 757133
Blocks: 1003095
As agreed on irc I'm going to ignore the review comment in comment 108 for now:

https://hg.mozilla.org/integration/fx-team/rev/f52ced52f369
For good measure, here is the try run for that patch:
https://hg.mozilla.org/integration/fx-team/rev/f52ced52f369

The gaia tests look iffy, but I'm fairly certain that this is unrelated to my patch.
Green try run for the root.js refactor:
https://tbpl.mozilla.org/?tree=Try&rev=9cce0eaa42a2
The root.js patch seems to have caused some failures that didn't show up in the tests, notably in bug 1002456. I've added a fix for it there, but since it's only a single line patch, I will also add it here in hope of getting it landed sooner.
Attachment #8415270 - Flags: review?(past)
Attachment #8415270 - Flags: review?(past) → review+
Attachment #8415270 - Attachment description: Fix → Fix for root.js refactor
This patch converts webbrowser.js into a CommonJS module. To accomplish this, I've extended the module API so that its also possible to define the root actor via a module.

If this approach meets with Panos' approval, I intend to do a similar refactor for testactor.js in the xpcshell tests. Turning testactor.js into an SDK module will allow it to be used in multiple servers loaded with different loaders, which is a requirement for bug 1003095.

I have a pending try run for this patch. Based on previous results it's not unlikely that we will see test failures on B2G, so we might have to iterate a couple of times before it can land:

  https://tbpl.mozilla.org/?tree=Try&rev=a5b308293e06
Attachment #8415312 - Flags: review?(past)
Attachment #8415337 - Flags: review?(past)
Green try run for the webbrowser.js patch:
https://tbpl.mozilla.org/?tree=Try&rev=33339dd2eec2
Comment on attachment 8415312 [details] [diff] [review]
Refactor webbrowser.js

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

I'm clearing the review request as it's impossible to test the patch for regressions in browser and add-on debugging probably due to bug 1002456. Please request review again once that is fixed and you have verified that the browser debugger works.
Attachment #8415312 - Flags: review?(past)
Comment on attachment 8415312 [details] [diff] [review]
Refactor webbrowser.js

Reflagging this patch for review assuming that the patch in bug 1002456 will stick.
Attachment #8415312 - Flags: review?(past)
Attachment #8415337 - Flags: review?(past)
Comment on attachment 8415312 [details] [diff] [review]
Refactor webbrowser.js

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

Looks good to me, but I've only tested on desktop (incl. chrome and add-on debugging). Please make sure that b2g and fennec are OK with this patch before landing.

::: toolkit/devtools/server/actors/webapps.js
@@ +15,2 @@
>  
> +let {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});

Is this necessary because webapps.js used to get promise loaded by webbrowser.js?

::: toolkit/devtools/server/actors/webbrowser.js
@@ +1459,5 @@
> +  handle.setRootActor(createRootActor);
> +};
> +
> +exports.unregister = function(handle) {
> +

For symmetry I would expect unregister() to call handle.setRootActor(null) or something. Is there any problem if you do that?
Attachment #8415312 - Flags: review?(past) → review+
Comment on attachment 8415337 [details] [diff] [review]
Refactor testactors.js

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

::: toolkit/devtools/server/tests/unit/testactors.js
@@ +125,5 @@
> +  handle.setRootActor(createRootActor);
> +};
> +
> +exports.unregister = function(handle) {
> +

Same comment about clearing the root actor factory here.
Attachment #8415337 - Flags: review?(past) → review+
mozilla-central changeset c390f04612a8 breaks the server on Fennec:

I/GeckoDump(31472): Error loading: chrome://browser/content/dbg-browser-actors.js:
I/GeckoDump(31472): at chrome://browser/content/dbg-browser-actors.js : 54
I/GeckoDump(31472): ReferenceError: BrowserTabList is not defined - @chrome://browser/content/dbg-browser-actors.js:54:1
I/GeckoDump(31472): loadSubScript@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:59:5
I/GeckoDump(31472): DS_addActors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:295:5
I/GeckoDump(31472): rd_start@chrome://browser/content/browser.js:7181:9
I/GeckoDump(31472): rd_init@chrome://browser/content/browser.js:7092:7
I/GeckoDump(31472): startup@chrome://browser/content/browser.js:381:5
I/GeckoDump(31472): onload@chrome://browser/content/browser.xul:1:1
Flags: needinfo?(ejpbruel)
I am going to look into this Fennec issue in bug 1010750.
Flags: needinfo?(ejpbruel)
Depends on: 1014141
We have the same problem in the webapp runtime. Filed bug 1014141.
(In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from comment #130)
> I am going to look into this Fennec issue in bug 1010750.

Hey Ryan, did you manage to fix this? And if not, are you still on it?
(In reply to Eddy Bruel [:ejpbruel] from comment #132)
> (In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from
> comment #130)
> > I am going to look into this Fennec issue in bug 1010750.
> 
> Hey Ryan, did you manage to fix this? And if not, are you still on it?

Yes, I've fixed the Fennec issue.  It's in fx-team now, and should be in m-c soon.
(In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from comment #133)
> (In reply to Eddy Bruel [:ejpbruel] from comment #132)
> > (In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from
> > comment #130)
> > > I am going to look into this Fennec issue in bug 1010750.
> > 
> > Hey Ryan, did you manage to fix this? And if not, are you still on it?
> 
> Yes, I've fixed the Fennec issue.  It's in fx-team now, and should be in m-c
> soon.

Thanks for confirming. And thank you for taking this off my plate. It's appreciated!
I'm not actively working on this anymore. We use the devtools loader now in the places that are important for worker debugging (most notably script.js). If there are still other places where the devtools loader is not being used, then someone else can pick that up.
Assignee: ejpbruel → nobody
Status: ASSIGNED → NEW
Product: Firefox → DevTools
Dave, do you think we can close this bug now?
Thanks
Flags: needinfo?(dcamp)
Keywords: leave-open
Whiteboard: [leave-open]
Yes, this has been recently really, completely finished by bug 1474980.
i.e. we no longer load server files via loadSubScript and always use Devtools module loader.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(dcamp)
Resolution: --- → FIXED
Assignee: nobody → ejpbruel
You need to log in before you can comment on or make changes to this bug.