1003095 - Implement a Worker Loader

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Eddy Bruel [:ejpbruel]]

We want the debugger server to be able to run in workers, where XPCOM is not available. To work around this, we are going to implement a CommonJS loader for workers, which does not depend on XPCOM, does not expose the chrome component to its modules, and provides any platform APIs we need as built-in modules. When running on the main thread, these platform APIs will be implemented as XPCOM components. When running on a worker thread, they will be implemented by the Worker Debugger API (see bug 757133).

The idea is that any server code that is able to run while it is being loaded by the worker loader on the main thread, will also be able to run on the worker thread. This allows us to get rid of all unnecessary XPCOM dependencies while still running on the main thread, and only then move everything over to the worker thread, which should make things much easier to debug.

To make sure people won't keep adding XPCOM dependencies to the code paths relevant for workers, I plan to refactor the xpcshell tests for those paths so that they are run twice: once for a server loaded with the devtools loader, and once for a server loaded with the worker loader.

Note that for all this to work, the code paths relevant for workers must load all their dependencies using the loader, and avoid the use of Cu.import and loadSubScript whenever possible, which makes this bug dependent on bug 859372.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Fix strict errors in the debugger server

Because we want to load the debugger server using different loaders, I tried to get rid of the dbg-server.jsm dependency in head_dbg.js. As it turns out, this causes strict warnings as errors to be turned on for several files. This patch fixes those strict errors.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Parameterize helper functions in head_dbg.js

To be able to run our tests against multiple servers, we need to make the server to be used a parameter of some of the helper functions in head_dbg.js. This patch takes care of that.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8417505 [details] [diff] [review]
Fix strict errors in the debugger server

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

There is a shorter form for dealing with the annoying warnings for "foo.bar may be undefined". Instead of:

"bar" in foo ? foo.bar : undefined

you can write:

foo.bar || undefined

::: testing/xpcshell/head.js
@@ +903,5 @@
>    } else if (typeof pattern == "object" && pattern) {
>      var matchers = [[p, pattern_matcher(pattern[p])] for (p in pattern)];
>      // Kludge: include 'length', if not enumerable. (If it is enumerable,
>      // we picked it up in the array comprehension, above.
> +    var ld = Object.getOwnPropertyDescriptor(pattern, 'length');

So the strict mode errors were useful for something after all! ;-)

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ +362,5 @@
> +// The do_check_* family of functions expect their last argument to be an
> +// optional stack object. Unfortunately, most tests actually pass a in a string
> +// containing an error message instead, which causes error reporting to break if
> +// strict warnings as errors is turned on. To avoid this, we wrap these
> +// functions here below to ensure the correct number of arguments is passed.

Can you add a TODO comment that this should be removed when bug 906232 is fixed?

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: Fix strict errors in the debugger server

New patch with review comments addressed

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Parameterize helper functions in head_dbg.js

Minor rebase for this patch.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor breakpoint tests to run against multiple loaders

This patch refactors the breakpoint tests so that they are run against two loaders. Right now both loaders are separate instances of the devtools loader but in a subsequent patch I will replace one of them with the custom worker loader I've been working on.

I eventually want to include more of the xpcshell tests, but getting the breakpoint tests to work seems like a reasonable first step.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Implement the worker loader

This patch implements the worker loader and runs it against the breakpoint tests. Note that this version of the loader still provides built-in modules that are based on the use of Components such as chrome, Services, Timers, etc. The next step will be to get rid of those dependencies for which I don't have replacement APIs on the worker side, and fix any code that breaks in the server as a result.

Note that this patch includes some minor changes to SDK code. I actually need to open a separate bug for those changes, but haven't gotten around to that yet (I'm currently stuck trying to get the SDK tests to run locally).

I've only included the SDK changes to the patch so that you can test it locally if you want to. Should you decide to r+ the patch, I will understand that to apply only to the non-SDK parts.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

It doesn't look like I will be able to get to these reviews before Monday.

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #8)
> It doesn't look like I will be able to get to these reviews before Monday.

That's ok. Thank you very much for giving me an ETA! :-)

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8419807 [details] [diff] [review]
Implement the worker loader

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

Nothing serious, just a few minor comments and ideas. I like how clean and thoroughly commented the worker loader is!

General style nit: we use /* */ style for long, descriptive, object or function comments and reserve inline style for shorter ones. Some tools can use the /** */ variety to generate documentation.

::: toolkit/devtools/sourcemap/SourceMap.jsm
@@ +1449,5 @@
>     */
>    function SourceNode(aLine, aColumn, aSource, aChunks, aName) {
>      this.children = [];
>      this.sourceContents = {};
> +    dump("VERRAAD VERRAAD!\n" + Error().stack + "\n");

Remove this.

::: toolkit/devtools/worker-loader.js
@@ +16,5 @@
> +// the use of Components and for which the worker debugger doesn't provide an
> +// alternative API, will be replaced by vacuous objects. Consequently, they can
> +// still be required, but any attempts to use them will lead to an exception.
> +
> +this.EXPORTED_SYMBOLS = ["WorkerDebuggerLoader", "worker"];

This file is a JSM, but it does not have the .jsm suffix, nor it follows the camel-case convention in its name. Why is that?

@@ +18,5 @@
> +// still be required, but any attempts to use them will lead to an exception.
> +
> +this.EXPORTED_SYMBOLS = ["WorkerDebuggerLoader", "worker"];
> +
> +// Some notes on modules ids and URLs:

Nit: "module"

@@ +21,5 @@
> +
> +// Some notes on modules ids and URLs:
> +//
> +// An id is either a relative id or an absolute id. An id is relative if and
> +// only if it start with a dot. An absolute id is a normalized id if and only if

Nit: "starts"

@@ +110,5 @@
> +//     Defaults to the empty map.
> +// - paths:
> +//     A map of paths to base URLs that will be used to resolve relative URLs to
> +//     absolute URLS. Defaults to the empty map.
> +// - resolve: 

Nit: trailing whitespace.

@@ +120,5 @@
> +  // Resolve the given relative URL to an absolute URL.
> +  function resolveURL(url) {
> +    let found = false;
> +    for (let [path, baseURL] of paths) {
> +      if (url.indexOf(path) === 0) {

if (url.startsWith(path)) {

@@ +131,5 @@
> +      throw new Error("can't resolve url " + url + "!");
> +    }
> +
> +    // If the url has no extension, use ".js" by default.
> +    return url + (url.match(/\.[^\\]*^/) === null ? ".js" : "");

This regular expression doesn't seem to work:

"foo/bar.js".match(/\.[^\\]*^/) === null

I'm guessing you probably meant to use /\.[^\\]*$/, but I still don't get the [^\\] part. Also note that this still fails for something like "foo/bar/". That is probably not important if we're going to be the only consumers of this loader, but in that case why not just write:

if (!url.endsWith(".js") {
  url += ".js";
}
return url;

@@ +173,5 @@
> +        // Failed to find the module to be required by id, so convert the id to
> +        // a URL and try again.
> +
> +        // If the id is relative, resolve it to an absolute id.
> +        if (id[0] === ".") {

Just a suggestion, but you could also use: if (id.startsWith("."))

@@ +222,5 @@
> +    };
> +  };
> +
> +  let createSandbox = options.createSandbox;
> +  let globals = options.globals || {};

Feel free to disregard if you disagree, but Object.create(null) instead of {} would be safer in all these initializations.

@@ +229,5 @@
> +  // Create the module cache by converting each entry in the map of built-in
> +  // modules to a module object with its exports property set to a frozen
> +  // version of the original entry.
> +  let modules = options.modules || {};
> +  Object.keys(modules).forEach(function (id) {

Not too important, but for..of should be faster AFAIK.

@@ +252,5 @@
> +this.WorkerDebuggerLoader = WorkerDebuggerLoader;
> +
> +// The default instance of the worker debugger loader is defined differently
> +// depending on whether it is loaded from the main thread or a worker thread.
> +if (typeof Components === "object") {

So where is the "else" branch? Or is it that you plan to make this branch throw when you get the loader ready for the worker tread?

@@ +295,5 @@
> +    // provide them in a way that does not depend on the use of Components.
> +    const { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {});;
> +    const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});;
> +    let SourceMap = {};
> +    Cu.import("resource://gre/modules/devtools/SourceMap.jsm", SourceMap);

Don't you want to use the source-map.js module here instead?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8419440 [details] [diff] [review]
Parameterize helper functions in head_dbg.js

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

This seems like too much churn for such a trivial change. Why not just use default values and contain the changes to head_dbg.js? Something like:

function initTestDebuggerServer(aServer = DebuggerServer) {...}

function addTestGlobal(aName, aServer = DebuggerServer) {...}

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8419442 [details] [diff] [review]
Refactor breakpoint tests to run against multiple loaders

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

This patch will need to change if you take my comments from the last review into account, but it also seems to needlessly move various global variables and functions to an inner scope. Without these changes the patch would have been significantly simpler.

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Parameterize helper functions in head_dbg.js

New patch with comments addressed.

Comment 14

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor breakpoint tests to run against multiple loaders

New patch with comments addressed.

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Create a whitelist for the chrome module

This patch creates a whitelist of modules that are allowed to require the built-in chrome module. If a module that's not in the list tries to require chrome, it gets a vacuous object instead.

Initially, the whitelist will contain all modules that are needed for worker debugging that currently depend on the chrome module. This will help to identify what modules we need to refactor.

By maintaining a whitelist, we can remove the dependency on chrome on a module by module basis. This is desirable because we want some test coverage for this as soon as possible: any attempts to readd chrome dependencies to modules from which I already removed them should cause test failures.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8422708 [details] [diff] [review]
Refactor breakpoint tests to run against multiple loaders

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

You didn't undo the move of the test functions to the inner scope, which would have cut down the patch size to a minimum. For this reason I only looked at the preamble in the tests (run_test/run_test_with_server) and assumed that everything else was just a move that can be undone. If not, I'll be happy to take another look at the updated patch.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8422793 [details] [diff] [review]
Create a whitelist for the chrome module

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

::: toolkit/devtools/worker-loader.js
@@ +201,5 @@
> +      // TODO: Remove this when the whitelist becomes empty
> +      if (id === "chrome" && chromeWhitelist.indexOf(requirer.id) < 0) {
> +        return { CC: undefined, Cc: undefined, ChromeWorker: undefined,
> +                 Cm: undefined, Ci: undefined, Cu: undefined, Cr: undefined,
> +                 components: undefined };

Is that really better than throwing? It seems to me that it will lead to confusing error messages for the uninitiated.

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #16)
> Comment on attachment 8422708 [details] [diff] [review]
> Refactor breakpoint tests to run against multiple loaders
> 
> Review of attachment 8422708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You didn't undo the move of the test functions to the inner scope, which
> would have cut down the patch size to a minimum. For this reason I only
> looked at the preamble in the tests (run_test/run_test_with_server) and
> assumed that everything else was just a move that can be undone. If not,
> I'll be happy to take another look at the updated patch.

I didn't undo them because the last test functions needs to call aCallback, which is an argument to the outer function. Simply indenting the test functions to make them part of the inner scope was less work than passing aCallback to every test function. Does that make sense, or would you still like me to do it differently?

Comment 19

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #17)
> Comment on attachment 8422793 [details] [diff] [review]
> Create a whitelist for the chrome module
> 
> Review of attachment 8422793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/worker-loader.js
> @@ +201,5 @@
> > +      // TODO: Remove this when the whitelist becomes empty
> > +      if (id === "chrome" && chromeWhitelist.indexOf(requirer.id) < 0) {
> > +        return { CC: undefined, Cc: undefined, ChromeWorker: undefined,
> > +                 Cm: undefined, Ci: undefined, Cu: undefined, Cr: undefined,
> > +                 components: undefined };
> 
> Is that really better than throwing? It seems to me that it will lead to
> confusing error messages for the uninitiated.

We don't want to throw here. We want top-level code like:

const { Cu, Ci } = require("chrome");

to work inside workers, as long as you don't actually use any code paths that *use* Cu and Ci.

Compare this with the much uglier:

try {
  var { Cu, Ci } = require("chrome");
} catch (e) {}

or:

if (hasChrome) {
  var { Cu, Ci } = require("chrome");
}

to guard against requiring chrome in a worker. I tried both approaches, but decided against them in the end.

Also note that actually using something like Cu in a worker will give you an error like 'Cu is not defined'. That's not *too* confusing imho (assuming you'rw aware that Cu might not always be available ofc).

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Eddy Bruel [:ejpbruel] from comment #18)
> I didn't undo them because the last test functions needs to call aCallback,
> which is an argument to the outer function. Simply indenting the test
> functions to make them part of the inner scope was less work than passing
> aCallback to every test function. Does that make sense, or would you still
> like me to do it differently?

I would have used a global gCallback next to the existing ones, but it's not really important since the bulk of the changes were mechanical. I was mostly trying to optimize the review response time ;-)

Oh, and you are totally right about avoiding try/catch clauses for require("chrome"). I forgot about that.

Comment 21

[Eddy Bruel [:ejpbruel]]

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

Comment 22

[Eddy Bruel [:ejpbruel]]

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

Comment 23

[Eddy Bruel [:ejpbruel]]

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

Note that I incorporated the changes suggested in comment 20 before landing.

Comment 24

[Eddy Bruel [:ejpbruel]]

The next patch depends on the fix in bug 1008199, so waiting for that to land.

Comment 25

[Eddy Bruel [:ejpbruel]]

Attachment: Make the xpcInspector a built-in module

The xpcinspector will be provided via Components on the main thread, and via WorkerDebuggerGlobalScope on worker threads, so the loader should provide it as a built-in module

Comment 26

[Girish Sharma [:Optimizer]]

Does it have to be a part of the built-in module ? because all Loader.jsm's built-in modules get loaded at startup.

Comment 27

[Eddy Bruel [:ejpbruel]]

(In reply to Girish Sharma [:Optimizer] from comment #26)
> Does it have to be a part of the built-in module ? because all Loader.jsm's
> built-in modules get loaded at startup.

Good point. It would be nice if we could lazily create xpcInspector in the loader, when the module is first required, but this is somewhat complicated by the fact that built-in modules are frozen when the loader is created.

Any suggestions to work around this?

Comment 28

[Girish Sharma [:Optimizer]]

Can't think of one straight ahead as this is not a jsm to be able to add it in paths instead.

Comment 29

[Eddy Bruel [:ejpbruel]]

Attachment: Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

We can't load JSM's in a worker, which means that we can't use XPCOMUtils.jsm. Since we only need 2 functions from this file, the easiest solution is to simply copy them over to DevToolsUtils.js, and require them from there.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9954f1b718d5
https://hg.mozilla.org/mozilla-central/rev/ef16253669f1
https://hg.mozilla.org/mozilla-central/rev/2018cc988098

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8423291 [details] [diff] [review]
Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

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

If these are the only uses of the XPCOMUtils lazy getters that we care about, then we could do something even simpler. NetworkMonitorManager and stringBundle are used in a single location each, so we could just inline the lazy getter there (and get rid of the L10N helper). Both of these code paths are not going to be taken in a worker.

|console| doesn't seem to be used anywhere in this module and is also being preloaded as a loader global, so you should be able to simply remove it.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8423284 [details] [diff] [review]
Make the xpcInspector a built-in module

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

The increased startup overhead is a real issue, but let's deal with that separately.

Comment 33

[Eddy Bruel [:ejpbruel]]

Attachment: Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

Comment 34

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor main.js to be worker friendly

This patch refactors main.js to be worker friendly by removing or lazifying any top-level references to chrome. This should make this code runnable inside a worker as long as we don't use any code paths that actually require chrome.

Comment 35

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8423988 [details] [diff] [review]
Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

diff -r 38b3aa17502e toolkit/devtools/DevToolsUtils.js
--- a/toolkit/devtools/DevToolsUtils.js	Fri May 16 18:35:06 2014 +0200
+++ b/toolkit/devtools/DevToolsUtils.js	Fri May 16 19:17:09 2014 +0200
@@ -346,8 +346,55 @@ exports.update = function update(aTarget
   for (let key in aNewAttrs) {
     let desc = Object.getOwnPropertyDescriptor(aNewAttrs, key);
 
     if (desc) {
       Object.defineProperty(aTarget, key, desc);
     }
   }
 }
+
+/**
+ * Defines a getter on a specified object that will be created upon first use.
+ *
+ * @param aObject
+ *        The object to define the lazy getter on.
+ * @param aName
+ *        The name of the getter to define on aObject.
+ * @param aLambda
+ *        A function that returns what the getter should return.  This will
+ *        only ever be called once.
+ */
+exports.defineLazyGetter = function defineLazyGetter(aObject, aName, aLambda) {
+  Object.defineProperty(aObject, aName, {
+    get: function () {
+      delete aObject[aName];
+      return aObject[aName] = aLambda.apply(aObject);
+    },
+    configurable: true,
+    enumerable: true
+  });
+};
+
+/**
+ * Defines a getter on a specified object for a module.  The module will not
+ * be imported until first use.
+ *
+ * @param aObject
+ *        The object to define the lazy getter on.
+ * @param aName
+ *        The name of the getter to define on aObject for the module.
+ * @param aResource
+ *        The URL used to obtain the module.
+ * @param aSymbol
+ *        The name of the symbol exported by the module.
+ *        This parameter is optional and defaults to aName.
+ */
+exports.defineLazyModuleGetter = function defineLazyModuleGetter(aObject, aName,
+                                                                 aResource,
+                                                                 aSymbol)
+{
+  this.defineLazyGetter(aObject, aName, function XPCU_moduleLambda() {
+    var temp = {};
+    Cu.import(aResource, temp);
+    return temp[aSymbol || aName];
+  });
+};
diff -r 38b3aa17502e toolkit/devtools/server/main.js
--- a/toolkit/devtools/server/main.js	Fri May 16 18:35:06 2014 +0200
+++ b/toolkit/devtools/server/main.js	Fri May 16 19:17:09 2014 +0200
@@ -44,17 +44,16 @@ this.dbg_assert = dbg_assert;
 // object usage
 Object.defineProperty(this, "Components", {
   get: function () require("chrome").components
 });
 
 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/XPCOMUtils.jsm");
 
 const LOG_PREF = "devtools.debugger.log";
 const VERBOSE_PREF = "devtools.debugger.log.verbose";
 dumpn.wantLogging = Services.prefs.getBoolPref(LOG_PREF);
 dumpv.wantVerbose =
   Services.prefs.getPrefType(VERBOSE_PREF) !== Services.prefs.PREF_INVALID &&
   Services.prefs.getBoolPref(VERBOSE_PREF);
 
@@ -80,23 +79,16 @@ let events = require("sdk/event/core");
 let {defer, resolve, reject, all} = require("devtools/toolkit/deprecated-sync-thenables");
 this.defer = defer;
 this.resolve = resolve;
 this.reject = reject;
 this.all = all;
 
 Cu.import("resource://gre/modules/devtools/SourceMap.jsm");
 
-XPCOMUtils.defineLazyModuleGetter(this, "console",
-                                  "resource://gre/modules/devtools/Console.jsm");
-
-XPCOMUtils.defineLazyGetter(this, "NetworkMonitorManager", () => {
-  return require("devtools/toolkit/webconsole/network-monitor").NetworkMonitorManager;
-});
-
 // XPCOM constructors
 const ServerSocket = CC("@mozilla.org/network/server-socket;1",
                         "nsIServerSocket",
                         "initSpecialConnection");
 const UnixDomainServerSocket = CC("@mozilla.org/network/server-socket;1",
                                   "nsIServerSocket",
                                   "initWithFilename");
 
@@ -595,16 +587,17 @@ var DebuggerServer = {
       childTransport.ready();
 
       aConnection.setForwarding(prefix, childTransport);
 
       dumpn("establishing forwarding for app with prefix " + prefix);
 
       actor = msg.json.actor;
 
+      let { NetworkMonitorManager } = require("devtools/toolkit/webconsole/network-monitor");
       netMonitor = new NetworkMonitorManager(aFrame, actor.actor);
 
       deferred.resolve(actor);
     }).bind(this);
     mm.addMessageListener("debug:actor", onActorCreated);
 
     let onMessageManagerDisconnect = DevToolsUtils.makeInfallible(function (subject, topic, data) {
       if (subject == mm) {
@@ -1276,23 +1269,20 @@ DebuggerServerConnection.prototype = {
           uneval(Object.keys(aPool._actors)));
   }
 };
 
 /**
  * Localization convenience methods.
  */
 let L10N = {
-
   /**
    * L10N shortcut function.
    *
    * @param string aName
    * @return string
    */
   getStr: function L10N_getStr(aName) {
     return this.stringBundle.GetStringFromName(aName);
-  }
+  },
+
+  stringBundle: Services.strings.createBundle(DBG_STRINGS_URI)
 };
-
-XPCOMUtils.defineLazyGetter(L10N, "stringBundle", function() {
-  return Services.strings.createBundle(DBG_STRINGS_URI);
-});

Comment 36

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8423988 [details] [diff] [review]
Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

diff -r 38b3aa17502e toolkit/devtools/DevToolsUtils.js
--- a/toolkit/devtools/DevToolsUtils.js	Fri May 16 18:35:06 2014 +0200
+++ b/toolkit/devtools/DevToolsUtils.js	Fri May 16 19:17:09 2014 +0200
@@ -346,8 +346,55 @@ exports.update = function update(aTarget
   for (let key in aNewAttrs) {
     let desc = Object.getOwnPropertyDescriptor(aNewAttrs, key);
 
     if (desc) {
       Object.defineProperty(aTarget, key, desc);
     }
   }
 }
+
+/**
+ * Defines a getter on a specified object that will be created upon first use.
+ *
+ * @param aObject
+ *        The object to define the lazy getter on.
+ * @param aName
+ *        The name of the getter to define on aObject.
+ * @param aLambda
+ *        A function that returns what the getter should return.  This will
+ *        only ever be called once.
+ */
+exports.defineLazyGetter = function defineLazyGetter(aObject, aName, aLambda) {
+  Object.defineProperty(aObject, aName, {
+    get: function () {
+      delete aObject[aName];
+      return aObject[aName] = aLambda.apply(aObject);
+    },
+    configurable: true,
+    enumerable: true
+  });
+};
+
+/**
+ * Defines a getter on a specified object for a module.  The module will not
+ * be imported until first use.
+ *
+ * @param aObject
+ *        The object to define the lazy getter on.
+ * @param aName
+ *        The name of the getter to define on aObject for the module.
+ * @param aResource
+ *        The URL used to obtain the module.
+ * @param aSymbol
+ *        The name of the symbol exported by the module.
+ *        This parameter is optional and defaults to aName.
+ */
+exports.defineLazyModuleGetter = function defineLazyModuleGetter(aObject, aName,
+                                                                 aResource,
+                                                                 aSymbol)
+{
+  this.defineLazyGetter(aObject, aName, function XPCU_moduleLambda() {
+    var temp = {};
+    Cu.import(aResource, temp);
+    return temp[aSymbol || aName];
+  });
+};
diff -r 38b3aa17502e toolkit/devtools/server/main.js
--- a/toolkit/devtools/server/main.js	Fri May 16 18:35:06 2014 +0200
+++ b/toolkit/devtools/server/main.js	Fri May 16 19:17:09 2014 +0200
@@ -44,17 +44,16 @@ this.dbg_assert = dbg_assert;
 // object usage
 Object.defineProperty(this, "Components", {
   get: function () require("chrome").components
 });
 
 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/XPCOMUtils.jsm");
 
 const LOG_PREF = "devtools.debugger.log";
 const VERBOSE_PREF = "devtools.debugger.log.verbose";
 dumpn.wantLogging = Services.prefs.getBoolPref(LOG_PREF);
 dumpv.wantVerbose =
   Services.prefs.getPrefType(VERBOSE_PREF) !== Services.prefs.PREF_INVALID &&
   Services.prefs.getBoolPref(VERBOSE_PREF);
 
@@ -80,23 +79,16 @@ let events = require("sdk/event/core");
 let {defer, resolve, reject, all} = require("devtools/toolkit/deprecated-sync-thenables");
 this.defer = defer;
 this.resolve = resolve;
 this.reject = reject;
 this.all = all;
 
 Cu.import("resource://gre/modules/devtools/SourceMap.jsm");
 
-XPCOMUtils.defineLazyModuleGetter(this, "console",
-                                  "resource://gre/modules/devtools/Console.jsm");
-
-XPCOMUtils.defineLazyGetter(this, "NetworkMonitorManager", () => {
-  return require("devtools/toolkit/webconsole/network-monitor").NetworkMonitorManager;
-});
-
 // XPCOM constructors
 const ServerSocket = CC("@mozilla.org/network/server-socket;1",
                         "nsIServerSocket",
                         "initSpecialConnection");
 const UnixDomainServerSocket = CC("@mozilla.org/network/server-socket;1",
                                   "nsIServerSocket",
                                   "initWithFilename");
 
@@ -595,16 +587,17 @@ var DebuggerServer = {
       childTransport.ready();
 
       aConnection.setForwarding(prefix, childTransport);
 
       dumpn("establishing forwarding for app with prefix " + prefix);
 
       actor = msg.json.actor;
 
+      let { NetworkMonitorManager } = require("devtools/toolkit/webconsole/network-monitor");
       netMonitor = new NetworkMonitorManager(aFrame, actor.actor);
 
       deferred.resolve(actor);
     }).bind(this);
     mm.addMessageListener("debug:actor", onActorCreated);
 
     let onMessageManagerDisconnect = DevToolsUtils.makeInfallible(function (subject, topic, data) {
       if (subject == mm) {
@@ -1276,23 +1269,20 @@ DebuggerServerConnection.prototype = {
           uneval(Object.keys(aPool._actors)));
   }
 };
 
 /**
  * Localization convenience methods.
  */
 let L10N = {
-
   /**
    * L10N shortcut function.
    *
    * @param string aName
    * @return string
    */
   getStr: function L10N_getStr(aName) {
     return this.stringBundle.GetStringFromName(aName);
-  }
+  },
+
+  stringBundle: Services.strings.createBundle(DBG_STRINGS_URI)
 };
-
-XPCOMUtils.defineLazyGetter(L10N, "stringBundle", function() {
-  return Services.strings.createBundle(DBG_STRINGS_URI);
-});

Comment 37

[Eddy Bruel [:ejpbruel]]

Attachment: Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

Wrong patch file

Comment 38

[Eddy Bruel [:ejpbruel]]

Attachment: Convert DevToolsExtensions.jsm into an SDK module

DevToolsExtensions.jsm is needed by script.js, but as you know by know, we can't use jsm's inside a worker. Before we can refactor script.js to be worker friendly, we thus need to convert DevToolsExtensions.jsm into an SDK module.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8424006 [details] [diff] [review]
Copy the lazy getter functions from XPCOMUtils.jsm to DevToolsUtils.js

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

r=me with comments addressed.

::: toolkit/devtools/DevToolsUtils.js
@@ +362,5 @@
> + * @param aLambda
> + *        A function that returns what the getter should return.  This will
> + *        only ever be called once.
> + */
> +exports.defineLazyGetter = function defineLazyGetter(aObject, aName, aLambda) {

There is no longer a need to copy these 2 functions, right?

::: toolkit/devtools/server/main.js
@@ +1283,5 @@
>    getStr: function L10N_getStr(aName) {
>      return this.stringBundle.GetStringFromName(aName);
> +  },
> +
> +  stringBundle: Services.strings.createBundle(DBG_STRINGS_URI)

So this makes calling createBundle eager from lazy and I think it will also happen for embeddings that don't even use DS__defaultAllowConnection. For that reason, I think it would be best to just create the bundle inside DS__defaultAllowConnection and use GetStringFromName directly, removing L10N completely. I looked at the platform code and CreateBundle doesn't do much. It's GetStringFromName that does the heavy lifting, so making it lazy wasn't buying us much anyway.

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8423996 [details] [diff] [review]
Refactor main.js to be worker friendly

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

::: toolkit/devtools/server/main.js
@@ +47,5 @@
>  });
>  
>  const DBG_STRINGS_URI = "chrome://global/locale/devtools/debugger.properties";
>  
> +DevToolsUtils.defineLazyGetter(this, "nsFile", () => {

Ah, so you use the copied functions in this patch! In that case ignore my previous comment.

It seems like although small patches are easier to review, breaking them in too small bits results in not being able to see the bigger picture. Just a thought to keep in mind.

@@ -56,5 @@
>  dumpv.wantVerbose =
>    Services.prefs.getPrefType(VERBOSE_PREF) !== Services.prefs.PREF_INVALID &&
>    Services.prefs.getBoolPref(VERBOSE_PREF);
>  
> -Cu.import("resource://gre/modules/devtools/deprecated-sync-thenables.js");

Are you sure about this? If I am not mistaken, some tests still fail when using async promises that the loader exposes by default. Make sure you get a green try run before landing this patch!

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8424014 [details] [diff] [review]
Convert DevToolsExtensions.jsm into an SDK module

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

Did you get a clean test run with this patch? It looks to me like it would fail at least these:

toolkit/devtools/tests/mochitest/test_devtools_extensions.html
addon-sdk/source/test/test-page-mod.js

Also, can we move DevToolsExtensions.js to toolkit/devtools/server as it is server-side code, and call it content-globals.js?

Comment 42

[Eddy Bruel [:ejpbruel]]

Attachment: Convert DevToolsExtensions.jsm into an SDK module

New patch with comments addressed.

Good catch on the test failures. I wrote that patch late at night, so I hadn't bothered to test it (what could possibly go wrong, right?) Sorry about that!

I'll make sure to get green try runs for each of these patches before landing them.

Comment 43

[Eddy Bruel [:ejpbruel]]

By the way, as it stands, the global cache in content-globals.js is not going to work inside a worker (we don't have Services, and we don't have Ci either). Is that going to be a problem? And if so, can we work around it?

Let's hash this out on irc tomorrow!

Comment 44

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor protocol.js to be worker friendly

Turns out that protocol.js doesn't actually depend on Cu, even though it requires it, so this is a trivial patch.

Comment 45

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor script.js to be worker friendly

The part that worries me here is that not having access to Ci will break all the object previewers in workers. How exactly are these object previewers used, and how does the observable behavior of the debugger change if they are not available?

Comment 46

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #45)
> Created attachment 8425142 [details] [diff] [review]
> Refactor script.js to be worker friendly
> 
> The part that worries me here is that not having access to Ci will break all
> the object previewers in workers. How exactly are these object previewers
> used, and how does the observable behavior of the debugger change if they
> are not available?

I should also point out that Nick assured me that NetUtil won't be used on any code paths used inside workers, so simply wrapping it inside a lazy getter should be enough.

Comment 47

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

"Assured" is a little strong. I am pretty sure, but I could be wrong!

The object previewers are what are used to show

  > a = { foo: 1, bar: 2 }
  Object { foo: 1, bar: 2 }

instead of:

  > a = { foo: 1, bar: 2 }
  [object Object]

In the webconsole and the variables view sidebar in the debugger.

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8425139 [details] [diff] [review]
Convert DevToolsExtensions.jsm into an SDK module

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

You forgot to update the test files again :-)

Comment 49

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Eddy Bruel [:ejpbruel] from comment #43)
> By the way, as it stands, the global cache in content-globals.js is not
> going to work inside a worker (we don't have Services, and we don't have Ci
> either). Is that going to be a problem? And if so, can we work around it?

content-globals.js contains a few helper functions for adding content globals created by jetpack add-ons that use the page-mod API. Since workers don't have access to the DOM, that API should be unavailable there.

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8425142 [details] [diff] [review]
Refactor script.js to be worker friendly

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

::: toolkit/devtools/server/actors/script.js
@@ +3842,5 @@
>        return false;
>      }
>  
>      let url;
> +    if (Ci && (aRawObj instanceof Ci.nsIDOMWindow) && aRawObj.location) {

Nit (here and elsewhere): the extra parentheses around instanceof are not necessary as it has higher precedence than &&.

::: toolkit/devtools/worker-loader.js
@@ +199,5 @@
>        //
>        // TODO: Remove this when the whitelist becomes empty
>        if (id === "chrome" && chromeWhitelist.indexOf(requirer.id) < 0) {
> +        const { Constructor: CC, classes: Cc, manager: Cm, interfaces: Ci,
> +                results: Cr, utils: Cu } = Components;

I don't understand this bit: why is this necessary?

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Eddy Bruel [:ejpbruel] from comment #45)
> The part that worries me here is that not having access to Ci will break all
> the object previewers in workers. How exactly are these object previewers
> used, and how does the observable behavior of the debugger change if they
> are not available?

What Nick said. I said on IRC that we should be fine because I think the problematic previewers for workers are those that concern DOM objects, which won't be present in a worker. Fingers crossed of course.

Comment 52

[Eddy Bruel [:ejpbruel]]

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

Comment 53

[Eddy Bruel [:ejpbruel]]

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

Comment 54

[Eddy Bruel [:ejpbruel]]

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

Comment 55

[Eddy Bruel [:ejpbruel]]

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

Comment 56

[Eddy Bruel [:ejpbruel]]

Attachment: Convert DevToolsExtensions.jsm into an SDK module

New patch with comments addressed.

This patch needs a separate review from somebody on the SDK team for the parts that touch SDK code.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/389fa7492938
https://hg.mozilla.org/mozilla-central/rev/8857da8762eb
https://hg.mozilla.org/mozilla-central/rev/a6dcd4c2fb6b
https://hg.mozilla.org/mozilla-central/rev/54526e9f8c31

Comment 58

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8426410 [details] [diff] [review]
Convert DevToolsExtensions.jsm into an SDK module

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

r+, I'd like to have the changes below about `devtoolsRequire` for consistency, but the rest I would say it's not strictly needed, as stated.

::: addon-sdk/source/lib/sdk/loader/sandbox.js
@@ +15,5 @@
>  const { getTabId, getTabForContentWindow } = require('../tabs/utils');
>  const { getInnerId } = require('../window/utils');
>  
> +const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const { require: devtoolsRequire } = devtools;

I would avoid this line. Just use `devtools.require('devtools/server/content-globals')`; we do the same when we load custom loader for unit test, so it will be consistent with the rest of our code.

::: addon-sdk/source/test/test-page-mod.js
@@ +29,5 @@
>  const { waitUntil } = require("sdk/test/utils");
>  const data = require("./fixtures");
>  
> +const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const { require: devtoolsRequire } = devtools;

Same as above.

::: toolkit/devtools/server/content-globals.js
@@ +1,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/. */
> +"use strict";
> +

Notice: all the comments above are intended in case of SDK module. This module is actually under devtools namespace, and they could have different coding style.
Plus, you just port a previous code: usually when we do that we also trying to align the old code to the current code style, however feel free to ignore it in this specific case, if you're not comfortable to modifying it.

@@ +5,5 @@
> +
> +const { Ci } = require("chrome");
> +const Services = require("Services");
> +
> +let globalsCache = {};

Why an object? Isn't a Map / WeakMap more indicate for that?

@@ +7,5 @@
> +const Services = require("Services");
> +
> +let globalsCache = {};
> +
> +exports.addContentGlobal = function(options) {

As our code guidelines says, this should be avoided: https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide#module-exports

You should have something like:

function addContentGlobal(options) {
  // ...
}
exports.addContentGlobal = addContentGlobal;

@@ +13,5 @@
> +    throw Error('Invalid arguments');
> +  }
> +  let cache = getGlobalCache(options['inner-window-id']);
> +  cache.push(options.global);
> +  return undefined;

I think this  is unnecessary; the function will returns `undefined` if there is no `return` statement.

@@ +16,5 @@
> +  cache.push(options.global);
> +  return undefined;
> +}
> +
> +exports.getContentGlobals = function(options) {

As above – coding style guide

@@ +23,5 @@
> +  }
> +  return Array.slice(globalsCache[options['inner-window-id']] || []);
> +}
> +
> +exports.removeContentGlobal = function(options) {

As above – coding style guide

@@ +38,5 @@
> +  return globalsCache[aInnerWindowID] = globalsCache[aInnerWindowID] || [];
> +}
> +
> +// when the window is destroyed, eliminate the associated globals cache
> +Services.obs.addObserver(function observer(subject, topic, data) {

Nit: you can probably use an arrow function here.
We try to use arrow function when is possible: https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide#functions

Comment 59

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #58)
> Notice: all the comments above are intended in case of SDK module. This

of course it was "below". :)

Comment 60

[Eddy Bruel [:ejpbruel]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #58)
> Comment on attachment 8426410 [details] [diff] [review]
> Convert DevToolsExtensions.jsm into an SDK module
> 
> Review of attachment 8426410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, I'd like to have the changes below about `devtoolsRequire` for
> consistency, but the rest I would say it's not strictly needed, as stated.
> 
> ::: addon-sdk/source/lib/sdk/loader/sandbox.js
> @@ +15,5 @@
> >  const { getTabId, getTabForContentWindow } = require('../tabs/utils');
> >  const { getInnerId } = require('../window/utils');
> >  
> > +const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> > +const { require: devtoolsRequire } = devtools;
> 
> I would avoid this line. Just use
> `devtools.require('devtools/server/content-globals')`; we do the same when
> we load custom loader for unit test, so it will be consistent with the rest
> of our code.

That line is there for a reason. If you use devtools.require the linker will treat it as an ordinary require and barf on it.

> 
> ::: addon-sdk/source/test/test-page-mod.js
> @@ +29,5 @@
> >  const { waitUntil } = require("sdk/test/utils");
> >  const data = require("./fixtures");
> >  
> > +const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> > +const { require: devtoolsRequire } = devtools;
> 
> Same as above.
> 
> ::: toolkit/devtools/server/content-globals.js
> @@ +1,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/. */
> > +"use strict";
> > +
> 
> Notice: all the comments above are intended in case of SDK module. This
> module is actually under devtools namespace, and they could have different
> coding style.
> Plus, you just port a previous code: usually when we do that we also trying
> to align the old code to the current code style, however feel free to ignore
> it in this specific case, if you're not comfortable to modifying it.
> 
> @@ +5,5 @@
> > +
> > +const { Ci } = require("chrome");
> > +const Services = require("Services");
> > +
> > +let globalsCache = {};
> 
> Why an object? Isn't a Map / WeakMap more indicate for that?
> 
> @@ +7,5 @@
> > +const Services = require("Services");
> > +
> > +let globalsCache = {};
> > +
> > +exports.addContentGlobal = function(options) {
> 
> As our code guidelines says, this should be avoided:
> https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide#module-exports
> 
> You should have something like:
> 
> function addContentGlobal(options) {
>   // ...
> }
> exports.addContentGlobal = addContentGlobal;
> 
> @@ +13,5 @@
> > +    throw Error('Invalid arguments');
> > +  }
> > +  let cache = getGlobalCache(options['inner-window-id']);
> > +  cache.push(options.global);
> > +  return undefined;
> 
> I think this  is unnecessary; the function will returns `undefined` if there
> is no `return` statement.
> 
> @@ +16,5 @@
> > +  cache.push(options.global);
> > +  return undefined;
> > +}
> > +
> > +exports.getContentGlobals = function(options) {
> 
> As above – coding style guide
> 
> @@ +23,5 @@
> > +  }
> > +  return Array.slice(globalsCache[options['inner-window-id']] || []);
> > +}
> > +
> > +exports.removeContentGlobal = function(options) {
> 
> As above – coding style guide
> 
> @@ +38,5 @@
> > +  return globalsCache[aInnerWindowID] = globalsCache[aInnerWindowID] || [];
> > +}
> > +
> > +// when the window is destroyed, eliminate the associated globals cache
> > +Services.obs.addObserver(function observer(subject, topic, data) {
> 
> Nit: you can probably use an arrow function here.
> We try to use arrow function when is possible:
> https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide#functions

Note that I didn't actually change anything in this file other than its name and the way we require things. I appreciate the thoroughness of your review, but I'm not going to address style nits for code that was already there ;-)

Comment 61

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor transport.js to be worker friendly

Comment 62

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor css-logic to be worker friendly

Comment 63

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor DevToolsUtils.js to be worker friendly

Comment 64

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor event-emitter.js to be worker friendly

Comment 65

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor object grips tests to run against the worker loader

Comment 66

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor stepping tests to run against the worker loader

Comment 67

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor the debugger server so that it works even when Services is not available

Comment 68

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Eddy Bruel [:ejpbruel] from comment #60)

> > > +const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> > > +const { require: devtoolsRequire } = devtools;
> > 
> > I would avoid this line. Just use
> > `devtools.require('devtools/server/content-globals')`; we do the same when
> > we load custom loader for unit test, so it will be consistent with the rest
> > of our code.
> 
> That line is there for a reason. If you use devtools.require the linker will
> treat it as an ordinary require and barf on it.

I see. In our test case we have such code but we also requires module that are in the SDK. In that case, because `devtools` variable is not used at all except to get the `require`, you could just have:

const { devtools: {require: devtoolsRequire} } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

> Note that I didn't actually change anything in this file other than its name
> and the way we require things.

I know, that's why I added in the first comment: 

"Plus, you just port a previous code: usually when we do that we also trying to align the old code to the current code style, however feel free to ignore it in this specific case"

;)

Comment 69

[Eddy Bruel [:ejpbruel]]

Green try run for the following patches:
- Convert DevToolsExtensions.jsm into an SDK module
- Refactor protocol.js to be worker friendly 
- Refactor main.js to be worker friendly

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

Note that Android failed to build here, but that seems unrelated.

Comment 70

[Eddy Bruel [:ejpbruel]]

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

This patch lands some changes into addon-sdk on fx-team, so these need to be synced with the sdk's git repo. Erik, what is the process for this?

Comment 71

[Eddy Bruel [:ejpbruel]]

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

Comment 72

[Eddy Bruel [:ejpbruel]]

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

Comment 73

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Eddy Bruel [:ejpbruel] from comment #70)
> https://hg.mozilla.org/integration/fx-team/rev/19295e500747
> 
> This patch lands some changes into addon-sdk on fx-team, so these need to be
> synced with the sdk's git repo. Erik, what is the process for this?

Land it in fx-team first.

Get the diff/patch, replace all of the "addon-sdk/source/" matches with "" (also remove diffs for files outside of this directory), `git apply <path-to-diff>`, `git commit -a --author="..." -m "..."`

or wait and I will do it before the next uplift.

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Tests in the Jetpack tree are no longer working now that fx-team has the patch and the jetpack tree doesn't. (See https://tbpl.mozilla.org/?tree=Jetpack&rev=5c6388ee3fb1 )

Error: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///tmp/tmpiMB4hu.mozrunner/extensions/[e-mail]!/bootstrap.js -> resource://extensions.modules.reading-data-example-at-jetpack-dot-mozillalabs-dot-com.commonjs.path/toolkit/loader.js -> resource://extensions.modules.reading-data-example-at-jetpack-dot-mozillalabs-dot-com.commonjs.path/sdk/loader/sandbox.js :: <TOP_LEVEL> :: line 20"  data: no] 

Erik, can you make sure you get this synced over today so tests work again?

Comment 75

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ab95884ddf6ff982213102a12204a4523e938e9b
Bug 1003095 - Convert DevToolsExtensions.jsm into an SDK module;r=past;r=ZER0

Comment 76

[Erik Vold [:erikvold] (please needinfo? me)]

https://github.com/mozilla/addon-sdk/commit/ab95884ddf6ff982213102a12204a4523e938e9b

Comment 77

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/19295e500747
https://hg.mozilla.org/mozilla-central/rev/03197ac125f3
https://hg.mozilla.org/mozilla-central/rev/f6b8338e3432

Comment 78

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8426900 [details] [diff] [review]
Refactor DevToolsUtils.js to be worker friendly

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

::: toolkit/devtools/DevToolsUtils.js
@@ +290,5 @@
>   *         True if it is safe to read properties from aObj, or false otherwise.
>   */
>  exports.isSafeJSObject = function isSafeJSObject(aObj) {
> +  // If we are running on a worker thread, Cu is not available. In this case,
> +  // we Always return false, just to be on the safe side.

Typo: Always -> always

Comment 79

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8426903 [details] [diff] [review]
Refactor object grips tests to run against the worker loader

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

::: toolkit/devtools/server/tests/unit/test_objectgrips-06.js
@@ +1,1 @@
> +/* Any copyright is dedica

Oops!

Comment 80

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8426977 [details] [diff] [review]
Refactor the debugger server so that it works even when Services is not available

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

Clearing review per our IRC discussion.

Comment 81

[Eddy Bruel [:ejpbruel]]

Attachment: Clean up how we provide several APIs

I wasn't quite happy with the last patch, so this patch cleans up that situation.

We still need to get rid of the dependency on Promise.jsm. I've filed a separate for that (see bug 1016301).

Comment 82

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8429211 [details] [diff] [review]
Clean up how we provide several APIs

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

::: toolkit/devtools/Loader.jsm
@@ +39,5 @@
>  /**
>   * Providers are different strategies for loading the devtools.
>   */
>  
> +let Timer = Cu.import("resource://gre/modules/Timer.jsm", {});

This seemed to fit better next to the other Cu.import statements, but oh well.

::: toolkit/devtools/event-emitter.js
@@ +149,5 @@
>  
>      if (logging) {
> +      let caller = isWorker ? undefined : components.stack.caller.caller;
> +      let func = caller ? caller.name : undefined;
> +      let path = caller ? caller.filename.split(/ -> /)[1] + ":" + caller.lineNumber : undefined;

Nit: I think it would be cleaner to write this as:

let caller, func, path;
if (!isWorker) {
  caller = ...
  func = ...
  path = ...
}

::: toolkit/devtools/worker-loader.js
@@ +333,2 @@
>          "chrome": chrome,
> +        "source-map": {},

Why is this here if you are adding a source-map path below?

@@ +337,5 @@
>          "": "resource://gre/modules/commonjs/",
>          "devtools": "resource:///modules/devtools",
>          "devtools/server": "resource://gre/modules/devtools/server",
>          "devtools/toolkit": "resource://gre/modules/devtools",
> +        "source-map": "resource://gre/modules/devtools/source-map.js",

You should remove the .js extension.

Comment 83

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/238b0a021b58
https://hg.mozilla.org/integration/fx-team/rev/cb60fbdfd0e1
https://hg.mozilla.org/integration/fx-team/rev/756f77a8b1b0

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/238b0a021b58
https://hg.mozilla.org/mozilla-central/rev/cb60fbdfd0e1
https://hg.mozilla.org/mozilla-central/rev/756f77a8b1b0

Comment 85

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/93c0524d1a41
https://hg.mozilla.org/integration/fx-team/rev/788564e45a81
https://hg.mozilla.org/integration/fx-team/rev/c97f33da608d
https://hg.mozilla.org/integration/fx-team/rev/845ea9c7b71c

Comment 86

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/788564e45a81
https://hg.mozilla.org/mozilla-central/rev/c97f33da608d
https://hg.mozilla.org/mozilla-central/rev/845ea9c7b71c

Comment 87

[Eddy Bruel [:ejpbruel]]

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

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7585a00f7f3e