596077 - Remove .packaging from "globals" (available to all modules)

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Brian Warner [:warner :bwarner]]

The manifest-enforcement code in 591515 can be easily bypassed by modules which manipulate "packaging.manifest", since currently all modules are given access to the "packaging" object.

This object should probably be removed from the "globals" set. In particular, it should probably be made available as part of the require("chrome") bundle, and not as a global.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

There is some add-on meta data that is contained in packaging that is both useful to developers and only reliably available in all environments from the packaging object, for example the add-on's current version. I logged a bug specifically around allowing access to version in bug 678556.

Comment 4

[Brian Warner [:warner :bwarner]]

require("self").version is the right way to do that. What other metadata from 'packaging' should we add to 'self'?

Comment 5

[Brian Warner [:warner :bwarner]]

(adding bug 678556 as a blocker, so that we provide self.version before removing packaging.version)

Comment 6

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

(In reply to Brian Warner [:warner :bwarner] from comment #4)
> require("self").version is the right way to do that. What other metadata
> from 'packaging' should we add to 'self'?

The two I could think of that could possibly be useful would be packaging.options.name and packaging.options.sdkVersion.
Everything else seems not useful for add-on devs or is based directly off of the add-on's 'id'...

Comment 7

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

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 8

[Brian Warner [:warner :bwarner]]

Attachment: test that 'packaging' is not present in globals

Comment 9

[Brian Warner [:warner :bwarner]]

Irakli's new loader removes 'packaging' from the default module globals. This patch asserts that it's really gone.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 570037 [details] [diff] [review]
test that 'packaging' is not present in globals

oops, I missed the existing test-globals.js . I'll make a new patch that puts these tests in there instead.

Comment 11

[Brian Warner [:warner :bwarner]]

Attachment: test that 'packaging' is not present in globals

ok, here's the better patch, updating the correct test

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 570043 [details] [diff] [review]
test that 'packaging' is not present in globals

>diff --git a/packages/api-utils/tests/test-globals.js b/packages/api-utils/tests/test-globals.js
>index 4ca1c04..0612779 100644
>--- a/packages/api-utils/tests/test-globals.js
>+++ b/packages/api-utils/tests/test-globals.js
>@@ -1,7 +1,18 @@
> exports.testGlobals = function(test) {
>+  // the only globals in module scope should be:
>+  //   module, exports, require, dump, console
>+  test.assertEqual(typeof(module), "object", "have 'module', good");
>+  test.assertEqual(typeof(exports), "object", "have 'exports', good");
>+  test.assertEqual(typeof(require), "function", "have 'require', good");
>+  test.assertEqual(typeof(dump), "function", "have 'dump', good");
>+  test.assertEqual(typeof(console), "object", "have 'console', good");

These work fine but would be better off using the new assertObject and assertFunction methods:

  test.assertObject(module, "have 'module', good");
  test.assertObject(exports, "have 'exports', good");
  test.assertFunction(require, "have 'require', good");
  test.assertFunction(dump, "have 'dump', good");
  test.assertObject(console, "have 'console', good");


>+  // in particular, these old globals should no longer be present
>+  test.assertEqual(typeof(packaging), "undefined", "no 'packaging', good");
>+  //test.assertEqual(typeof(memory), "undefined", "no 'memory', good"); // bug 620559

This last one is the opposite of an uncommented test a few lines down:

>   test.assertEqual(typeof memory, 'object', 'should define memory');

We should consolidate these, and we also now have test.expectFail, which is better than a commented-out test, since it generates a test result (which even mentions the expected failure, although the message is not as clear as it could be):

  test.expectFail(function() test.assertEqual(typeof(memory), "undefined", "no 'memory', good"));


Finally, note test.assertUndefined, which can replace these last two test.assertEqual calls, except that you must dereference a property to get the special `undefined` value to pass it, which in this case means defining a reference to the global object outside of the test function and then dereferencing the globals via its properties, so something like:

  Object.defineProperty(this, "global", { value: this });
  
  exports.testGlobals = function(test) {

    ...
  
    // in particular, these old globals should no longer be present
    test.assertUndefined(global.packaging, "no 'packaging', good");
    test.expectFail( // bug 620559
      function() test.assertUndefined(global.memory, "no 'memory', good")
    );
  
    ...

  };

Comment 13

[Brian Warner [:warner :bwarner]]

Landed, with myk's recommendations, in https://github.com/mozilla/addon-sdk/commit/94a2193dfd23c5f2685d3002f162b535c1d6b487