Closed Bug 752468 Opened 12 years ago Closed 12 years ago

Old jetpack versions cause zombie compartments (gecko regression)

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox15 + fixed
firefox16 --- fixed

People

(Reporter: till, Assigned: khuey)

References

()

Details

(Keywords: regression, Whiteboard: [MemShrink][see comment 19])

Attachments

(1 file)

The problem and thus the STR are identical to the one described in bug 751420:

1. install Visual Hashing from https://addons.mozilla.org/en-US/firefox/addon/visual-hashing/
2. open a tab to some unique page
3. close the tab
4. open about:compartments
5. notice the page is now in the ghost windows table
6. disable Visual Hashing
7. refresh about:compartments, notice the ghost window table is now empty

As the addon has last been updated in December, it'll probably be fixed by re-building with the latest Addon SDK.
Component: General → Add-ons
Product: Core → Tech Evangelism
QA Contact: general → addons
Version: Trunk → unspecified
Kyle's bug 752877 fixed this, AFAWCT.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
In recent Nightlies, the addon causes gost windows for every closed tab again. Same STRs as in comment 0 apply.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If this add-on is JS-only, I'd be very interested to know how it's causing ghost windows.  Kris, do you think you can investigate?
Seems like there's something going on with my profile: Installing the addon by itself in a fresh profile doesn't cause ghost windows.

I'll investigate more.
Nevermind, the addon does cause ghost windows in a clean profile after all.

Here's what's getting printed on closing a tab:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid1-nziezrc5syx4ww-at-jetpack-api-utils-lib/observer-service.js", line 176, in 
    this.callback(subject, data);
  File "resource://jid1-nziezrc5syx4ww-at-jetpack-api-utils-lib/content/worker.js", line 524, in _documentUnloa
    this._workerCleanup();
  File "resource://jid1-nziezrc5syx4ww-at-jetpack-api-utils-lib/content/worker.js", line 555, in _workerCleanu
    this._contentWorker._destructor();
  File "resource://jid1-nziezrc5syx4ww-at-jetpack-api-utils-lib/content/worker.js", line 322, in _destructo
    delete sandbox.__proto__;
  File "resource://jid1-nziezrc5syx4ww-at-jetpack-api-utils-lib/content/content-proxy.js", line 647, in 
    return name in obj || name in overload || name == "__isWrappedProxy";
TypeError: can't access dead object
Hm. That's an ancient Jetpack version. My guess is that it's the same issue as bug 751420 comment 12. Regardless, I don't have any reason to suspect that this is anything new.
> My guess is that it's the same issue as bug 751420 comment 12.

If so, shouldn't this be fixed in nightlies?  (Maybe it is; I'm not totally clear from the bug.)
> > My guess is that it's the same issue as bug 751420 comment 12.
> 
> If so, shouldn't this be fixed in nightlies?  (Maybe it is; I'm not totally
> clear from the bug.)

It's not. In fact, this *seems* to be a regression. At least, I didn't experience memory leaks and increases in GC and CC times like what I had for the last couple of days of using the addon before. I can try finding a regression range if that's of interest and there's a possibility of it turning up new information.
(In reply to Justin Lebar [:jlebar] from comment #8)
> If so, shouldn't this be fixed in nightlies?  (Maybe it is; I'm not totally
> clear from the bug.)

As I understand it, the fix for that issue was in SDK code rather than in Firefox, so unless add-ons upgrade their included SDK, they'll still be affected.
> As I understand it, the fix for that issue was in SDK code rather than in Firefox, so unless add-ons 
> upgrade their included SDK, they'll still be affected.

Although the issue was fixed in the SDK (actually, it was never broken in the latest version of the SDK), bug 752877, a Gecko change, theoretically fixed things for old versions of the SDK.
It may have fixed that particular bug for older SDK versions (I don't know), but the issue of the broken wrapper throwing an exception during cleanup seems to be the same, from the stack trace above.
We should get a handle, then, on which versions of the Jetpack SDK are still affected.
Blocks: 749526
I can do that, but there are known memory leaks and zombie compartment issues as far as 1.6 (http://blog.mozilla.org/addons/2012/05/07/memshrink-progress-leaky-add-ons-and-older-sdk-versions/)
(In reply to Kris Maglione [:kmag] from comment #14)
> I can do that, but there are known memory leaks and zombie compartment
> issues as far as 1.6
> (http://blog.mozilla.org/addons/2012/05/07/memshrink-progress-leaky-add-ons-
> and-older-sdk-versions/)

Right, but aiui all of the remaining leaks are of one or two Jetpack-internal compartments.  What's particularly concerning here is the fact that every site visited results in a zombie compartment.
(In reply to Justin Lebar [:jlebar] from comment #16)
> Right, but aiui all of the remaining leaks are of one or two
> Jetpack-internal compartments.  What's particularly concerning here is the
> fact that every site visited results in a zombie compartment.

That doesn't surprise me much, since the add-on in question runs a page mod on every website the user visits. I suppose that bug 752877 fixes this in the Jetpack version that was current at the time it was discovered, but this version is ancient and its cleanup code is still being short circuited. I suppose we could try to work around it, if common add-ons are still using the version, or other add-ons are similarly affected, but I really doubt it's worth the time.
> I suppose that bug 752877 fixes this in the Jetpack version that was current at the time 
> it was discovered

bug 752877 fixed old versions of Jetpack as I understand.  Current versions were not affected.

We need to understand what versions of Jetpack are affected here before we can understand whether it's worth worrying about.
I just confirmed that this is a somewhat recent regression in the browser itself, not Jetpack.

The last known good version is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-27-03-05-15-mozilla-central/
or, more precisely (and a bit weirdly), the next inbound:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-27-04-02-22-mozilla-inbound/

The first known bad version is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-28-09-30-39-mozilla-central/

That leads to a regression range of:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=133aa3a2ef0a&tochange=79262a88881d

( And please, whatever you do, do *not* find out that my commit in that range is to blame ;) )
After looking at the commits in that range, sadly I'd put my money on my own commit after all. It's the only one that's touching CCWs in that range.
(In reply to Justin Lebar [:jlebar] from comment #18)
> bug 752877 fixed old versions of Jetpack as I understand.  Current versions
> were not affected.

It fixed some old versions. I don't know that it fixed all of them. I agree that we need to know what versions are affected, but as it seems to be the result of a regression, I'll wait until that's fixed to check.
Whoa, big increase in ghost windows on 5/28.

http://goo.gl/mpOHv
Component: Add-ons → General
Product: Tech Evangelism → Core
QA Contact: addons → general
Summary: Visual Hashing addon causes every website visited to be retained as a ghost window → Old jetpack versions cause zombie compartments (gecko regression)
Whiteboard: [MemShrink] → [MemShrink][see comment 19]
This needs to track FF15; it undoes much of the gains we saw with respect to leaked windows.
I just tested with my try build[1] for bug 758278 and that doesn't trigger the regression. So it seems like that change didn't cause it - at least not in itself.

[1]: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-7e351cd119ba/try-macosx64/
So have we decided that this is different from my stuff?
Ok, I bisected and this is different, but I'm still the one that broke it ;-)

Patch coming up.
Assignee: nobody → khuey
Blocks: 751999
Status: REOPENED → ASSIGNED
Attached patch PatchSplinter Review
This re-removes code that was accidentally added back in in Bug 751999.
Attachment #630024 - Flags: review?(jst)
Attachment #630024 - Flags: review?(jst) → review+
https://hg.mozilla.org/mozilla-central/rev/a7a905fd70d5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment on attachment 630024 [details] [diff] [review]
Patch

Virtually no risk, fixes leaks.
Attachment #630024 - Flags: approval-mozilla-aurora?
Attachment #630024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
khuey, can you land this on Aurora? :)
I've tried to land twice, only to see that Aurora was burning.  I'll try to land today.

*sees mid-air collision report*

Thanks Andrew!
You need to log in before you can comment on or make changes to this bug.