Bug 1309351 - Part 2: Use a shared global sandbox and simple module resolution for built-in modules. r=ochameau, a=ritu
authorKris Maglione <maglione.k@gmail.com>
Fri, 14 Oct 2016 06:14:10 +0100
changeset 348746 39997b314c291878b4214931ae62b216836b1129
parent 348745 49a8e8f20df9d637cec8fc772c317b88474b9123
child 348747 15fa0eb880c7df116f70c9d5f07e559d2e003d45
push id6546
push userryanvm@gmail.com
push dateThu, 27 Oct 2016 00:42:23 +0000
treeherdermozilla-beta@075f767684d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersochameau, ritu
bugs1309351
milestone50.0
Bug 1309351 - Part 2: Use a shared global sandbox and simple module resolution for built-in modules. r=ochameau, a=ritu This takes another 21% off the Jetpack test suite run time for me. MozReview-Commit-ID: 1U2lq4PN21w
addon-sdk/source/app-extension/bootstrap.js
addon-sdk/source/lib/sdk/addon/bootstrap.js
addon-sdk/source/lib/sdk/system/globals.js
addon-sdk/source/lib/toolkit/loader.js
addon-sdk/source/test/test-content-events.js
--- a/addon-sdk/source/app-extension/bootstrap.js
+++ b/addon-sdk/source/app-extension/bootstrap.js
@@ -290,18 +290,21 @@ function loadSandbox(uri) {
       CC: bind(CC, Components), components: Components,
       ChromeWorker: ChromeWorker });
   };
   scriptLoader.loadSubScript(uri, sandbox, 'UTF-8');
   return sandbox;
 }
 
 function unloadSandbox(sandbox) {
-  if ("nukeSandbox" in Cu)
-    Cu.nukeSandbox(sandbox);
+  if ("nukeSandbox" in Cu) {
+    try {
+      Cu.nukeSandbox(sandbox);
+    } catch (e) {}
+  }
 }
 
 function setTimeout(callback, delay) {
   let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   timer.initWithCallback({ notify: callback }, delay,
                          Ci.nsITimer.TYPE_ONE_SHOT);
   return timer;
 }
@@ -338,16 +341,17 @@ function nukeModules() {
   }
   // Direct links to sandboxes should be removed too
   for (let key in loader.sandboxes) {
     let sandbox = loader.sandboxes[key];
     delete loader.sandboxes[key];
     // Bug 775067: From FF17 we can kill all CCW from a given sandbox
     unloadSandbox(sandbox);
   }
+  unloadSandbox(loader.sharedGlobalSandbox);
   loader = null;
 
   // both `toolkit/loader` and `system/xul-app` are loaded as JSM's via
   // `cuddlefish.js`, and needs to be unloaded to avoid memory leaks, when
   // the addon is unload.
 
   unloadSandbox(cuddlefishSandbox.loaderSandbox);
 
--- a/addon-sdk/source/lib/sdk/addon/bootstrap.js
+++ b/addon-sdk/source/lib/sdk/addon/bootstrap.js
@@ -149,21 +149,31 @@ Bootstrap.prototype = {
     return new Promise(resolve => {
       const { loader } = this;
       if (loader) {
         this.loader = null;
         unload(loader, reason);
 
         setTimeout(() => {
           for (let uri of Object.keys(loader.sandboxes)) {
-            Cu.nukeSandbox(loader.sandboxes[uri]);
+            try {
+              Cu.nukeSandbox(loader.sandboxes[uri]);
+            } catch (e) {
+              // This will throw for shared sandboxes.
+            }
             delete loader.sandboxes[uri];
             delete loader.modules[uri];
           }
 
+          try {
+            Cu.nukeSandbox(loader.sharedGlobalSandbox);
+          } catch (e) {
+            Cu.reportError(e);
+          }
+
           resolve();
         }, 1000);
       }
       else {
         resolve();
       }
     });
   }
--- a/addon-sdk/source/lib/sdk/system/globals.js
+++ b/addon-sdk/source/lib/sdk/system/globals.js
@@ -30,10 +30,17 @@ Object.defineProperty(exports, 'define',
   // variables remain accessible.
   configurable: true,
   get: function() {
     let sandbox = this;
     return function define(factory) {
       factory = Array.slice(arguments).pop();
       factory.call(sandbox, sandbox.require, sandbox.exports, sandbox.module);
     }
-  }
+  },
+  set: function(value) {
+    Object.defineProperty(this, 'define', {
+      configurable: true,
+      enumerable: true,
+      value,
+    });
+  },
 });
--- a/addon-sdk/source/lib/toolkit/loader.js
+++ b/addon-sdk/source/lib/toolkit/loader.js
@@ -330,24 +330,25 @@ const load = iced(function load(loader, 
     get Components() {
       // Expose `Components` property to throw error on usage with
       // additional information
       throw new ReferenceError(COMPONENT_ERROR);
     }
   });
 
   let sandbox;
-  if (loader.sharedGlobalSandbox &&
+  if ((loader.useSharedGlobalSandbox || isSystemURI(module.uri)) &&
       loader.sharedGlobalBlocklist.indexOf(module.id) == -1) {
     // Create a new object in this sandbox, that will be used as
     // the scope object for this particular module
     sandbox = new loader.sharedGlobalSandbox.Object();
     // Inject all expected globals in the scope object
     getOwnIdentifiers(globals).forEach(function(name) {
       descriptors[name] = getOwnPropertyDescriptor(globals, name)
+      descriptors[name].configurable = true;
     });
     Object.defineProperties(sandbox, descriptors);
   }
   else {
     sandbox = Sandbox({
       name: module.uri,
       prototype: Object.create(globals, descriptors),
       wantXrays: false,
@@ -621,16 +622,24 @@ Loader.resolveURI = resolveURI;
 // with it during link time.
 const Require = iced(function Require(loader, requirer) {
   let {
     modules, mapping, resolve: loaderResolve, load,
     manifest, rootURI, isNative, requireMap,
     requireHook
   } = loader;
 
+  if (isSystemURI(requirer.uri)) {
+    // Built-in modules don't require the expensive module resolution
+    // algorithm used by SDK add-ons, so give them the more efficient standard
+    // resolve instead.
+    isNative = false;
+    loaderResolve = Loader.resolve;
+  }
+
   function require(id) {
     if (!id) // Throw if `id` is not passed.
       throw Error('You must provide a module name when calling require() from '
                   + requirer.id, requirer.uri);
 
     if (requireHook) {
       return requireHook(id, _require);
     }
@@ -752,16 +761,19 @@ const Require = iced(function Require(lo
       // If not found in the map, not a node module, and wasn't able to be
       // looked up, it's something
       // found in the paths most likely, like `sdk/tabs`, which should
       // be resolved relatively if needed using traditional resolve
       if (!requirement) {
         requirement = isRelative(id) ? Loader.resolve(id, requirer.id) : id;
       }
     }
+    else if (modules[id]) {
+      uri = requirement = id;
+    }
     else if (requirer) {
       // Resolve `id` to its requirer if it's relative.
       requirement = loaderResolve(id, requirer.id);
     }
     else {
       requirement = id;
     }
 
@@ -928,45 +940,43 @@ function Loader(options) {
       get: function() {
         return builtinModuleExports[id];
       }
     });
 
     modules[uri] = freeze(module);
   });
 
-  let sharedGlobalSandbox;
-  if (sharedGlobal) {
-    // Create the unique sandbox we will be using for all modules,
-    // so that we prevent creating a new comportment per module.
-    // The side effect is that all modules will share the same
-    // global objects.
-    sharedGlobalSandbox = Sandbox({
-      name: "Addon-SDK",
-      wantXrays: false,
-      wantGlobalProperties: [],
-      invisibleToDebugger: options.invisibleToDebugger || false,
-      metadata: {
-        addonID: options.id,
-        URI: "Addon-SDK"
-      },
-      prototype: options.sandboxPrototype || {}
-    });
-  }
+  // Create the unique sandbox we will be using for all modules,
+  // so that we prevent creating a new comportment per module.
+  // The side effect is that all modules will share the same
+  // global objects.
+  let sharedGlobalSandbox = Sandbox({
+    name: "Addon-SDK",
+    wantXrays: false,
+    wantGlobalProperties: [],
+    invisibleToDebugger: options.invisibleToDebugger || false,
+    metadata: {
+      addonID: options.id,
+      URI: "Addon-SDK"
+    },
+    prototype: options.sandboxPrototype || {}
+  });
 
   // Loader object is just a representation of a environment
   // state. We freeze it and mark make it's properties non-enumerable
   // as they are pure implementation detail that no one should rely upon.
   let returnObj = {
     destructor: { enumerable: false, value: destructor },
     globals: { enumerable: false, value: globals },
     mapping: { enumerable: false, value: mapping },
     // Map of module objects indexed by module URIs.
     modules: { enumerable: false, value: modules },
     metadata: { enumerable: false, value: metadata },
+    useSharedGlobalSandbox: { enumerable: false, value: !!sharedGlobal },
     sharedGlobalSandbox: { enumerable: false, value: sharedGlobalSandbox },
     sharedGlobalBlocklist: { enumerable: false, value: sharedGlobalBlocklist },
     sharedGlobalBlacklist: { enumerable: false, value: sharedGlobalBlocklist },
     // Map of module sandboxes indexed by module URIs.
     sandboxes: { enumerable: false, value: {} },
     resolve: { enumerable: false, value: resolve },
     // ID of the addon, if provided.
     id: { enumerable: false, value: options.id },
@@ -996,16 +1006,18 @@ function Loader(options) {
     returnObj.requireMap = { enumerable: false, value: requireMap };
     returnObj.rootURI = { enumerable: false, value: addTrailingSlash(rootURI) };
   }
 
   return freeze(Object.create(null, returnObj));
 };
 Loader.Loader = Loader;
 
+var isSystemURI = uri => /^resource:\/\/(gre|devtools|testing-common)\//.test(uri);
+
 var isJSONURI = uri => uri.endsWith('.json');
 var isJSMURI = uri => uri.endsWith('.jsm');
 var isJSURI = uri => uri.endsWith('.js');
 var isAbsoluteURI = uri => uri.startsWith("resource://") ||
                            uri.startsWith("chrome://") ||
                            uri.startsWith("file://");
 var isRelative = id => id.startsWith(".");
 
--- a/addon-sdk/source/test/test-content-events.js
+++ b/addon-sdk/source/test/test-content-events.js
@@ -61,22 +61,17 @@ exports["test dead object errors"] = fun
   let cleanup = () => system.off("console-api-log-event", onMessage);
   let fail = (reason) => {
     cleanup();
     assert.fail(reason);
   }
 
   loader.unload();
 
-  // in order to get a dead object error on this module, we need to nuke
-  // the relative sandbox; unload the loader is not enough
-  let url = Object.keys(loader.sandboxes).
-    find(url => url.endsWith("/sdk/content/events.js"));
-
-  nuke(loader.sandboxes[url]);
+  nuke(loader.sharedGlobalSandbox);
 
   system.on("console-api-log-event", onMessage, true);
 
   openBrowserWindow().
     then(closeWindow).
     then(() => assert.pass("checking dead object errors")).
     then(cleanup).
     then(done, fail);