Closed Bug 614348 Opened 9 years ago Closed 9 years ago

browser/fuel/test/browser_ApplicationQuitting.js causes ASSERTION: XPConnect is being called on a scope without a 'Components' property


(Firefox Graveyard :: Panorama, defect)

Not set


(blocking2.0 betaN+)

Tracking Status
blocking2.0 --- betaN+


(Reporter: dbaron, Assigned: raymondlee)



(Keywords: assertion, memory-leak, Whiteboard: [qa-])


(1 file)

chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationQuitting.js causes a bunch of:

ASSERTION: XPConnect is being called on a scope without a 'Components' property!

The first assertion is an attempt to call the observer TabView__initFrame in browser.js (sourced from browser-tabview.js).  As far as I can tell, this observer is per-window and never removed, which makes it a bad memory leak (unless the Services code does something magical I don't know about).

The second assertion is probably an attempt to call the similar observer in UI_init in browser/base/content/tabview/ui.js.  Again, looks like a bad memory leak unless there's magic I'm not aware of.

Then these two keep alternating for quite a while.  (I'm looking at tinderbox output with the debugging patch in bug 510489.)
blocking2.0: --- → ?
This also causes a bunch of assertions during actual shutdown of the browser-chrome tests.
This seems to be very test-specific. We should fix it, but it doesn't block release.
blocking2.0: ? → -
No, I nominated it because it's the panorama code that we ship that's leaking observers; the test is just triggering that leak.
blocking2.0: - → ?
Please forgive my ignorance, but are you referring to this chunk in TabView._initFrame?

      function observer(subject, topic, data) {
        if (topic == "quit-application-requested") {
          let data = (self.isVisible() ? "true" : "false");
          self._sessionstore.setWindowValue(window, self._visibilityID, data);
      Services.obs.addObserver(observer, "quit-application-requested", false);

... and is the fix to add a removeObserver call when we're done?
Attached patch v1Splinter Review
Add code to remove the observers.
Attachment #498279 - Flags: review?(ian)
Attachment #498279 - Flags: feedback?(dbaron)
Attachment #498279 - Attachment is patch: true
Attachment #498279 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 498279 [details] [diff] [review]

Yeah, this looks pretty much like what I'd expect a fix for this to look like.  Was there something in particular you wanted me to look at?
Attachment #498279 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 498279 [details] [diff] [review]

Looks good to me, assuming Dao's happy with the location of the addition to browser.js.
Attachment #498279 - Flags: review?(ian)
Attachment #498279 - Flags: review+
Attachment #498279 - Flags: feedback?(dao)
Attachment #498279 - Flags: feedback?(dao) → feedback+
Assignee: nobody → raymond
blocking2.0: ? → betaN+
Comment on attachment 498279 [details] [diff] [review]

I'm not sure if this needs approval... I suppose it's a test fix, but it involves changes to non-test code.
Attachment #498279 - Flags: approval2.0?
Comment on attachment 498279 [details] [diff] [review]

Nevermind... now it's blocking.

I've sent it to try; will probably land it this afternoon.
Attachment #498279 - Flags: approval2.0?
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.