Bug 1309351: Part 2 - Use a shared global sandbox and simple module resolution for built-in modules. r?ochameau draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 13 Oct 2016 06:42:48 +0100
changeset 424680 292e833393cf7c262e6ad446476d8fca2213959d
parent 424679 75fa66d6f9a98d4bc3aa0776848e8eea7227bddf
child 533730 ffb828830ac25a586cbe06d81cef3ea1895698ce
push id32217
push usermaglione.k@gmail.com
push dateThu, 13 Oct 2016 05:44:55 +0000
reviewersochameau
bugs1309351
milestone52.0a1
Bug 1309351: Part 2 - Use a shared global sandbox and simple module resolution for built-in modules. r?ochameau This takes another 21% off the Jetpack test suite run time for me. MozReview-Commit-ID: 1U2lq4PN21w
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/lib/sdk/addon/bootstrap.js
+++ b/addon-sdk/source/lib/sdk/addon/bootstrap.js
@@ -152,21 +152,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
@@ -469,24 +469,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,
@@ -730,16 +731,21 @@ 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)) {
+    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);
     }
@@ -861,16 +867,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;
     }
 
@@ -1034,45 +1043,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 },
@@ -1102,16 +1109,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);