Jetpack 1.12 - toolkit/loader, globals are broken



Add-on SDK
6 years ago
3 years ago


(Reporter: simon.lachinger, Assigned: irakli)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Sabayon Chrome/23.0.1271.91 Safari/537.11

Steps to reproduce:

I tried to reuse javascript code spread over several files from the pre-jetpack addon implementation which share a common namespace:
com.mycompany = { .. }

According to the docs, using the "toolkit/loader" module and the "com" object as a global, this should work similar to this:

/* main.js */
var com = {};
var loader = require('toolkit/loader');
var packaging = require('@loader/options');
var custom_options = loader.override(
    { globals: { 'com': com,
                 console: console,
                 module: module,
                 exports: exports,
                 require: require,
                 dump: dump
// now create the custom loader
var custom_loader = loader.Loader(custom_options);
var custom_require = loader.Require( custom_loader, module );
// load the files required by the addon

Actual results:

The 'com' object was not passed on to the code loaded with custom_require(). Debugging showed that it gets lost during the creation of the sandbox:

This snipped is taken from the load() function in sdk/lib/toolkit/loader.js:

  let sandbox = sandboxes[module.uri] = Sandbox({
    name: module.uri,
    // Get an existing module sandbox, if any, so we can reuse its compartment
    // when creating the new one to reduce memory consumption.
    sandbox: sandboxes[keys(sandboxes).shift()],
    // We expose set of properties defined by `CommonJS` specification via
    // prototype of the sandbox. Also globals are deeper in the prototype
    // chain so that each module has access to them as well.
    prototype: create(globals, descriptor({
      require: require,
      module: module,
      exports: module.exports
    wantXrays: false

The object.create() method used hides the 'com' object in the prototype. Which it shouldn't actually do, however if I dump it it is gone:
dump(JSON.stringify(create(globals, descriptor({
      require: require,
      module: module,
      exports: module.exports

Also, if I dump 'com' in one of the loaded files it is 'undefined'.

Expected results:

Globals should work as described here:

The "com" object in the sandbox used for loading of the files should be a reference to the one defined in main.js

This actually worked with addon-sdk-1.8 but wasn't a documented feature at the time. Now it is documented, but fails.


6 years ago
Priority: -- → P3
Assignee: nobody → rFobic
Hey Irakli, you've had this bug assigned to you for quite some time, are you planning to work on it?  and how important is it?
Flags: needinfo?(rFobic)
Created attachment 8555581 [details] [review]
Test for the described issues that is passing.
Flags: needinfo?(rFobic)
Attachment #8555581 - Flags: review?(evold)
simon it's being a while & I don't know if this bug was resolved since you've tried it or if there was something wrong with your code. Anyhow I created test case (attached to the bug) which does passes with a current master. Although it's not a exactly the code you've written since:

1. JSON.parse(JSON.stringify(packaging)) is not supported since there is no guarantees that packaging is serializable.
2. `module`, `require` and `exports` objects are not supposed to be shared across modules, so putting them into global is not valid use of the API. That being said I highly doubt that was an issue here.

As of the following comment:

> The object.create() method used hides the 'com' object in the prototype. Which it shouldn't actually do, however if I dump it it is gone:

Shared globals are intentionally put into prototype chain instead of copying it's properties onto every single sandbox that way changes to them are also shared across all modules. Either way that can't be an issues.

I suggest we close it as works for me after test case lands.
Attachment #8555581 - Flags: review?(evold) → review+

Comment 4

3 years ago
Commits pushed to master at
Create a test case to cover scenario from the bug 827792.
Merge pull request #1845 from Gozala/bug/loader-globals@827792

Bug 827792 - Create a test case to cover scenario from the bug r=erikvold
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.