677768 - require("timers") in Aurora causes error

Status: RESOLVED FIXED

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

Description

[Shane Tomlinson [:stomlinson]]

Attachment: A unit test for "timers.js"

Attached is an empty unit test suite that only 'const timers = require("timers");'

I have also tried 'let timers = require("timers");"

When trying to run this unit test on OSX 10.6 using Aurora, I get an error.

Command to run:
cfx test -b /Applications/Aurora.app/Contents/MacOS/firefox -f timers

Result:
Using binary at '/Applications/Aurora.app/Contents/MacOS/firefox'.
Using profile at '/var/folders/HE/HE7h0MIEHKaxtWwqohgNfk+++TI/-Tmp-/tmpRjtJRq.mozrunner'.
Running tests on Firefox 7.0a2/Gecko 7.0a2 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under Darwin/x86_64-gcc3.
Traceback (most recent call last):
  File "<string>", line 2, in 
TypeError: can't change object's extensibility
No tests were run
FAIL
Total time: 2.602509 seconds
Program terminated unsuccessfully.

Comment 1

[Dave Townsend [:mossop]]

Not sure why our tests wouldn't be catching this problem, important to figure this out though.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Fix module.exports freeze

We recently added this Object.freeze(exports) call in bug 672199.
But in Aurora and Nightly we are seing this error:
  TypeError: can't change object's extensibility
In most cases it is about calling Object.freeze on an object that is already frozen. Newer version of the platform throw this kind of error on such case.

If we execute the following script in Nightly:
  var a = {}; Object.freeze(a); Object.freeze(a);
We do not get any exception, so it seems to be limited to complex code using sandboxes ...

Irakli: here is a patch to simply trap this exception. 
Do you have some idea on why it is failing in our code but not in simple test?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 552500 [details] [diff] [review]
Fix module.exports freeze

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

It looks like that issues is not that double freeze fails but a cross compartment freeze throwing errors as reported in:
https://bugzilla.mozilla.org/show_bug.cgi?id=674195

So timers module fails because module.exports is an object from a different compartment. I would suggest to do following instead

Object.isPrototypeOf(module.exports) ? Object.freeze(module.exports) : Object.freeze(Object.create(module.exports))

::: packages/api-utils/lib/securable-module.js
@@ +367,5 @@
>           var preeval_exports = sandbox.getProperty("exports");
>           self.modules[path] = sandbox.getProperty("exports");
>           sandbox.evaluate(moduleContents);
> +         // If exports is already frozen, Object.freeze throw an exception,
> +         // we need to catch it for addon-kit/timers module

I'm not sure that's true, at least it's not supposed to do that and if it does we deal with some platform bug that requires a fix.

My quick test shows that:

(function() { "use strict"; return Object.freeze(Object.freeze({})) })()  

works just fine.

Also if that's really about double freeze than we should do following instead

if (!Object.isFrozen(module.exports))
   Object.freeze(module.exports)

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> So timers module fails because module.exports is an object from a different
> compartment. I would suggest to do following instead
> 
> Object.isPrototypeOf(module.exports) ? Object.freeze(module.exports) :
> Object.freeze(Object.create(module.exports))

Sorry I meant Object.prototype.isPrototypeOf

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Fix freeze using Object.create and add an unit test for timers.

You are right, this issue is related to bug 674195.

The following script, throw the same exception:
Object.freeze(Components.utils.evalInSandbox("({})", Components.utils.Sandbox("http://mozilla.org")))

Here is a patch that use your suggested code and I added a test for `timers`.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 553423 [details] [diff] [review]
Fix freeze using Object.create and add an unit test for timers.

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

r+ as long as you address the comment.

::: packages/api-utils/lib/securable-module.js
@@ +374,5 @@
> +           "if (typeof module.exports === 'object')\n" +
> +           "  Object.prototype.isPrototypeOf(module.exports) ? " +
> +           "    Object.freeze(module.exports) : " +
> +           "    Object.freeze(Object.create(module.exports));");
> +

Well you need to assign 

Object.freeze(Object.create(module.exports)) to module.exports 

otherwise that statement is pointless.

so I think it will be better of

if (typeof(module.exports) === 'object')
   module.exports = Object.prototype.isPrototypeOf(module.exports) ?
   Object.freeze(module.exports) :
   Object.freeze(Object.create(module.exports));

Comment 7

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/0a05f6d537a02862e72b5cd3e0f7da0ee485d56a

Comment 8

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

(Not an issue in 1.1, targeted at Firefox 7, so retriaging accordingly.)

Comment 9

[Ed Lee :Mardak]

I seemed to have run into this issue on Firefox Release (6) using sdk1.0 and sdk1.1b1 with this line:

const {setTimeout} = require("timers");

exception:
File "<string>", line 2, in
TypeError: can't change object's extensibility

This also happens on 7 and 8 but not 9.

I've worked around this issue by doing require("timer") instead of "timers" with a "s".

Comment 10

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

(In reply to Edward Lee :Mardak from comment #9)
> I seemed to have run into this issue on Firefox Release (6) using sdk1.0 and
> sdk1.1b1 with this line:
> 
> const {setTimeout} = require("timers");

I can't reproduce this on Firefox 6 (Mac) with SDK 1.0.  To test, I downloaded the SDK 1.0 tarball, expanded it, and activated it.  Then I used `cfx init` to create a basic `foo` addon and replaced the contents of its main.js script with:

  const { setTimeout } = require("timers");
  console.log(setTimeout);
  setTimeout(function() console.log("timed out!"), 10);

Finally, I called `cfx run`, pointing it to the Firefox 6 binary, and it behaved as expected:

(addon-sdk-1.0)mykbook:foo myk$ cfx run --binary /Applications/Firefox\ 6.app/
Using binary at '/Applications/Firefox 6.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/bN/bN4Z5w6UEdC60cFgLX32NU+++TI/-Tmp-/tmpIwCao0.mozrunner'.
info: function setTimeout(callback, delay) {
    return makeTimer(Ci.nsITimer.TYPE_ONE_SHOT, callback, TimeoutCallback, delay, Array.slice(arguments, 2));
}
info: timed out!
OK
Total time: 3.381664 seconds
Program terminated successfully.


Mardak: can you provide any more info about how you tested that might explain this discrepancy?

Comment 11

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

However, I do see the problem on Firefoxes Release (6), Beta (7), Aurora (8), and Nightly (9) with SDKs Stabilization (1.1) and Development (1.2).  So it looks like this is a regression between SDKs 1.0 and 1.1 in conjunction with a platform change between Firefoxes 5 and 6.  And that means we need to land this patch on stabilization.

Mardak: I'd still be interested to hear if you can reproduce with Firefox 6 + SDK 1.0.  If so, that means we missed this incompatibility when testing those products, and it raises the issue of whether or not to push an SDK 1.0.1 to fix the problem (probably not, at this point, but worth considering).

Comment 12

[Ed Lee :Mardak]

I can't seem to reproduce the issue with SDK 1.0 on Fx6, but I can hit the error with SDK 1.1b1 on Fx6:

1) cfx init
2) edit lib/main.js to have just "const {setTimeout} = require("timers");"
3) cfx xpi (2x for id then .xpi)
4) install xpi in Firefox Release

Error: An exception occurred.
Traceback (most recent call last):
  File "<string>", line 2, in 
TypeError: can't change object's extensibility

I'm pretty sure I was running cfx xpi with 1.0 last week testing on Release, but there might have been some caching going on.

Comment 13

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

Cherry-picked to stabilization (1.1):

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