Closed Bug 603906 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: irakli)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Blocks: 604641
OS: Linux → All
Hardware: x86 → All
Component: Jetpack SDK → XPConnect
Product: Mozilla Labs → Core
QA Contact: jetpack-sdk → xpconnect
Target Milestone: -- → mozilla2.0b7
Version: unspecified → Trunk
Blocking a b7 blocker.
blocking2.0: --- → ?
How do I run this test? Is there a cfx command?
This one shows up when you run |cfx test -F panel|.
(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.
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
blocking2.0: ? → beta7+
Assignee: nobody → mrbkap
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.
Assignee: mrbkap → rFobic
No longer blocks: 604641
Component: XPConnect → Jetpack SDK
Product: Core → Mozilla Labs
QA Contact: xpconnect → jetpack-sdk
Target Milestone: mozilla2.0b7 → 0.10
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.
Assignee: rFobic → nobody
Component: Jetpack SDK → XPConnect
Product: Mozilla Labs → Core
QA Contact: jetpack-sdk → xpconnect
Target Milestone: 0.10 → mozilla2.0b7
blocking2.0: beta7+ → ---
Component: XPConnect → Jetpack SDK
Product: Core → Mozilla Labs
QA Contact: xpconnect → jetpack-sdk
Target Milestone: mozilla2.0b7 → --
That worked, no longer blocking b7.
Assignee: nobody → rFobic
Target Milestone: -- → 0.10
Attachment #487545 - Flags: review? → review?(myk)
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.
Blocks: 604641
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.
Attachment #487545 - Flags: review?(myk) → review+
It is called but later on I think. In any case we won't have remove soon.
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?
Keywords: checkin-needed
Attached patch v2Splinter Review
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 ?
Attachment #487545 - Attachment is obsolete: true
Attachment #488845 - Flags: review?
Attachment #488845 - Flags: review? → review+
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: 0.10 → 1.0b1
You need to log in before you can comment on or make changes to this bug.