608866 - traits tests throw "TypeError: self.constructor is undefined" with fix for bug 596031

Status: RESOLVED FIXED

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

Description

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

Attachment: patch that fixes problem but causes another

On a Firefox trunk build with the fix for bug 596031 applied (like these tryserver builds: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-85abae22b304/tryserver-linux/>), the traits tests throw "TypeError: self.constructor is undefined":

--------------------------------------------------------------------------------
(addon-sdk)myk@myk:~/Projects/addon-sdk/packages/jetpack-core$ cfx test --binary=~/Downloads/firefox/firefox -F test-traits.js
Using binary at '/home/myk/Downloads/firefox/firefox'.
Using profile at '/tmp/tmpXUAoUT.mozrunner'.

(firefox-bin:25985): GLib-WARNING **: g_set_prgname() called multiple times
.................error: TEST FAILED: test-traits.test:instances must not be hackable (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 255, in
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 280, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 298, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 63, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-traits.js", line 132, in
    let i1 = Class();
  File "resource://jetpack-core-jetpack-core-lib/traits.js", line 118, in Trait
    return self.constructor.apply(self, arguments) || self._public;
TypeError: self.constructor is undefined
...............error: An exception occurred.
Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 255, in
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 280, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 298, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 63, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-traits.js", line 389, in
    let (i = 0, sub = Sub()) {
  File "resource://jetpack-core-jetpack-core-lib/traits.js", line 118, in Trait
    return self.constructor.apply(self, arguments) || self._public;
TypeError: self.constructor is undefined

32 of 34 tests passed.
FAIL
Total time: 1.583697 seconds
Program terminated unsuccessfully.
--------------------------------------------------------------------------------


The attached patch fixes the problem but causes another (presumably because the code no longer does the additional checks in traits/core::create):

--------------------------------------------------------------------------------
(addon-sdk)myk@myk:~/Projects/addon-sdk/packages/jetpack-core$ cfx test --binary=~/Downloads/firefox/firefox -F test-traits.js
Using binary at '/home/myk/Downloads/firefox/firefox'.
Using profile at '/tmp/tmpWFUoDE.mozrunner'.

(firefox-bin:26729): GLib-WARNING **: g_set_prgname() called multiple times
.........................error: TEST FAILED: test-traits.test:required (failure)
error: fail: should throw when creating instance with required properties
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 255, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 280, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 298, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 63, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-traits.js", line 253, in anonymous
    test.fail('should throw when creating instance with required properties');
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();
............
37 of 38 tests passed.
FAIL
Total time: 1.456443 seconds
Program terminated unsuccessfully.
--------------------------------------------------------------------------------

Comment 1

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

Attachment: patch that works around problem

Here's a patch that works around the problem.  It just copies the create() function from traits/core to traits, so the traits sandbox uses its own Object.create() function instead of the one from the other sandbox.

We can use a patch like this (with a bit of cleanup) to work around the problem on the SDK side of things if we want to land the fix for bug 596031 in 4.0b7 but not fix the regression it causes until 4.0b8.

Comment 2

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

BTW I can't reproduce this on mac, while I can on ubuntu so it's seems that it's a platform specific misbehavior.

Comment 3

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

Ok I have investigated into this bug little bit more and js engine misbehaves badly on linux. I guess it's a bug with compartments. Here is the test case: 

js> var sandbox = Components.utils.Sandbox('http://mozilla.com')
js> Components.utils.evalInSandbox('function boom(proto, map) Object.create(proto, map)', sandbox)
js> Object.prototype.constructor
function Object() {
    [native code]...}
js> sandbox.boom(Object.prototype, {}).constructor
js> typeof sandbox.boom(Object.prototype, {}).constructor
"undefined"
js> Object.prototype.constructor
js> typeof Object.prototype.constructor
"undefined"

One very important detail is that Object.create not only ignored constructor but it also removed it form Object.prototype in other compartment, which may break lot's of completely unrelated code.

Comment 4

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

I made a bug report for this: https://bugzilla.mozilla.org/show_bug.cgi?id=608959

Comment 5

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

Attachment: patch v1: works around problem

Here's an updated patch that implements the simplest workaround along with some comments referencing the bugs in question.  With this patch, |cfx test| in addon-kit no longer dies on "TypeError: self.constructor is undefined" without running any tests.

Comment 6

[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 7

[Atul Varma [:atul]]

Comment on attachment 488511 [details] [diff] [review]
patch v1: works around problem

I'm reassigning this review to myself because it's really high priority.

Comment 8

[Atul Varma [:atul]]

Comment on attachment 488511 [details] [diff] [review]
patch v1: works around problem

Pushed this workaround as commit 8e7f6881856d7796894548d87d6325f6af7642fa. Not marking the bug as fixed, though, since the patch is just a workaround.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Atul, can you test if the patch in bug 608959 helped here?

Comment 10

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

(In reply to comment #9)
> Atul, can you test if the patch in bug 608959 helped here?

I tested with a tip build now that bug 608959's patch has landed in mozilla-central, and it does resolve the problem, so all tests pass even when I remove the workaround.

I'm going to leave the workaround in place for the time being so that tests continue to pass against Firefox 4.0b7.  Once b8 comes out and we shift to using it as our reference build, we can remove the workaround.