Last Comment Bug 752468 - Old jetpack versions cause zombie compartments (gecko regression)
: Old jetpack versions cause zombie compartments (gecko regression)
Status: RESOLVED FIXED
[MemShrink][see comment 19]
: regression
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on: 751466
Blocks: LeakyAddons ZombieCompartments hueyfix 749526 751999
  Show dependency treegraph
 
Reported: 2012-05-07 05:52 PDT by Till Schneidereit [:till]
Modified: 2012-06-13 07:02 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Patch (1.00 KB, patch)
2012-06-04 18:17 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jst: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Till Schneidereit [:till] 2012-05-07 05:52:37 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2012-05-07 08:21:41 PDT
1k users.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-15 16:28:01 PDT
Kyle's bug 752877 fixed this, AFAWCT.
Comment 3 Till Schneidereit [:till] 2012-05-31 08:49:38 PDT
In recent Nightlies, the addon causes gost windows for every closed tab again. Same STRs as in comment 0 apply.
Comment 4 Justin Lebar (not reading bugmail) 2012-05-31 22:30:21 PDT
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?
Comment 5 Till Schneidereit [:till] 2012-06-01 05:57:36 PDT
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.
Comment 6 Till Schneidereit [:till] 2012-06-01 06:14:20 PDT
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
Comment 7 Kris Maglione [:kmag] 2012-06-01 12:51:10 PDT
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.
Comment 8 Justin Lebar (not reading bugmail) 2012-06-03 14:56:09 PDT
> 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.)
Comment 9 Till Schneidereit [:till] 2012-06-03 17:34:07 PDT
> > 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.
Comment 10 Kris Maglione [:kmag] 2012-06-03 17:40:25 PDT
(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.
Comment 11 Justin Lebar (not reading bugmail) 2012-06-03 21:11:12 PDT
> 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.
Comment 12 Kris Maglione [:kmag] 2012-06-04 10:03:51 PDT
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.
Comment 13 Justin Lebar (not reading bugmail) 2012-06-04 10:05:41 PDT
We should get a handle, then, on which versions of the Jetpack SDK are still affected.
Comment 14 Kris Maglione [:kmag] 2012-06-04 10:12:28 PDT
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/)
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 10:13:53 PDT
We fixed those, no?
Comment 16 Justin Lebar (not reading bugmail) 2012-06-04 10:14:29 PDT
(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.
Comment 17 Kris Maglione [:kmag] 2012-06-04 13:51:55 PDT
(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.
Comment 18 Justin Lebar (not reading bugmail) 2012-06-04 14:00:40 PDT
> 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.
Comment 19 Till Schneidereit [:till] 2012-06-04 15:07:13 PDT
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 ;) )
Comment 20 Till Schneidereit [:till] 2012-06-04 15:23:14 PDT
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.
Comment 21 Kris Maglione [:kmag] 2012-06-04 15:25:35 PDT
(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.
Comment 22 Justin Lebar (not reading bugmail) 2012-06-04 16:10:34 PDT
Whoa, big increase in ghost windows on 5/28.

http://goo.gl/mpOHv
Comment 23 Justin Lebar (not reading bugmail) 2012-06-04 16:11:48 PDT
This needs to track FF15; it undoes much of the gains we saw with respect to leaked windows.
Comment 24 Till Schneidereit [:till] 2012-06-04 16:23:29 PDT
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/
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 16:37:48 PDT
So have we decided that this is different from my stuff?
Comment 26 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 18:05:17 PDT
Ok, I bisected and this is different, but I'm still the one that broke it ;-)

Patch coming up.
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 18:17:12 PDT
Created attachment 630024 [details] [diff] [review]
Patch

This re-removes code that was accidentally added back in in Bug 751999.
Comment 28 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 20:40:27 PDT
https://hg.mozilla.org/mozilla-central/rev/a7a905fd70d5
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-04 20:41:06 PDT
Comment on attachment 630024 [details] [diff] [review]
Patch

Virtually no risk, fixes leaks.
Comment 30 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-13 01:10:59 PDT
khuey, can you land this on Aurora? :)
Comment 31 Andrew McCreight [:mccr8] 2012-06-13 06:19:23 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/df5c5255c62c
Comment 32 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-13 07:02:15 PDT
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!

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