603906 - "have to add the panel" exception in "test-panel.test:destruct before removed"

Status: RESOLVED FIXED

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

Description

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

On the latest Firefox 4 nightly, "test-panel.test:destruct before removed" fails because of the exception "Error: You have to add the panel via require('panel').add() before you can show it.":

error: TEST FAILED: test-panel.test:destruct before removed (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://addon-kit-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://addon-kit-jetpack-core-lib/unit-test.js", line 255, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://addon-kit-jetpack-core-lib/unit-test.js", line 280, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://addon-kit-jetpack-core-lib/unit-test.js", line 298, in start
    this.test.testFunction(this);
  File "resource://addon-kit-jetpack-core-lib/unit-test.js", line 63, in runTest
    test(runner);
  File "resource://addon-kit-addon-kit-tests/test-panel.js", line 160, in 
    panel.show();
  File "resource://addon-kit-addon-kit-lib/panel.js", line 144, in show
    throw new Error(ERR_ADD);
Error: You have to add the panel via require('panel').add() before you can show it.

Comment 1

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

I see this on Windows, Mac, and Linux.  It was masked for a while by bug 607077, but it's back now that that bug has been fixed.

Comment 2

[Andreas Gal :gal]

Blocking a b7 blocker.

Comment 3

[Andreas Gal :gal]

How do I run this test? Is there a cfx command?

Comment 4

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

This one shows up when you run |cfx test -F panel|.

Comment 5

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

(In reply to comment #4)
> This one shows up when you run |cfx test -F panel|.

Erm: you run that command in the packages/addon-kit/ directory.

Comment 6

[Andreas Gal :gal]

This is currently blocked by the this._browsers issue, so we are going to have to fix that first before we can diagnose much further.

TypeError: this._browsers is undefined

Comment 7

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

Irakli dug into this further, and it turns out to be an SDK issue rather than a platform issue.  The traits module uses Object.freeze to prevent Trait instances from changing; then the test tries to change a Trait instance.

That works in Firefox 4.0b6 because Object.freeze apparently didn't actually work in that release.  On trunk, however, it does work, and this test fails.

Irakli is going to look into this further and figure out the fix on the SDK side of things, but this is not a 4.0b7 blocker.

Comment 8

[Andreas Gal :gal]

This is still showing up as blocking b7. I will try to move back to xpconnect, unset the flag (the flag is hidden right now), and then move it back.

Comment 9

[Andreas Gal :gal]

That worked, no longer blocking b7.

Comment 10

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

Attachment: v1

http://github.com/mozilla/addon-sdk/pull/23

Comment 11

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

Comment on attachment 487545 [details] [diff] [review]
v1

I don't quite understand this change.  The name of the test is "destruct before remove", but this change causes the panel to be removed before it is destroyed, since unloading the module causes it to remove all panels before calling their destructors, no?  If so, then it makes the test no longer test what it claims to test.

Comment 12

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

Comment on attachment 487545 [details] [diff] [review]
v1

> I don't quite understand this change.  The name of the test is "destruct before
> remove", but this change causes the panel to be removed before it is destroyed,
> since unloading the module causes it to remove all panels before calling their
> destructors, no?  If so, then it makes the test no longer test what it claims
> to test.

Irakli pointed out http://github.com/Gozala/addon-sdk/blob/master/packages/jetpack-core/lib/content/symbiont.js#L104, which shows that unload actually does call panel destructors without removing the panels.  So this patch does work, although it's strange that remove() isn't getting called.  It seems like it should.

Comment 13

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

It is called but later on I think. In any case we won't have remove soon.

Comment 14

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

Irakli: can you land this patch or, if changes are needed, because there's a problem with this patch, submit a new patch with the changes?

Comment 15

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

Attachment: v2

I have added changes to the pull request:
https://github.com/mozilla/addon-sdk/pull/23

Also attaching patch here. According to this link, I'm under impression that reviews may be requested just by pull requests: http://etherpad.mozilla.com:9000/jetpack-git

Is this true ?

Comment 16

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