Closed
Bug 752468
Opened 12 years ago
Closed 12 years ago
Old jetpack versions cause zombie compartments (gecko regression)
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: till, Assigned: khuey)
References
()
Details
(Keywords: regression, Whiteboard: [MemShrink][see comment 19])
Attachments
(1 file)
1.00 KB,
patch
|
jst
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Component: General → Add-ons
Product: Core → Tech Evangelism
QA Contact: general → addons
Version: Trunk → unspecified
Updated•12 years ago
|
Blocks: LeakyAddons
Comment 1•12 years ago
|
||
1k users.
Updated•12 years ago
|
Blocks: ZombieCompartments
Depends on: 751466
Comment 2•12 years ago
|
||
Kyle's bug 752877 fixed this, AFAWCT.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 3•12 years ago
|
||
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 → ---
Comment 4•12 years ago
|
||
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?
Reporter | ||
Comment 5•12 years ago
|
||
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.
Reporter | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
> 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.)
Reporter | ||
Comment 9•12 years ago
|
||
> > 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•12 years ago
|
||
(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•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
We should get a handle, then, on which versions of the Jetpack SDK are still affected.
Comment 14•12 years ago
|
||
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/)
Assignee | ||
Comment 15•12 years ago
|
||
We fixed those, no?
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
> 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.
Reporter | ||
Comment 19•12 years ago
|
||
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 ;) )
Reporter | ||
Comment 20•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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]
Comment 23•12 years ago
|
||
This needs to track FF15; it undoes much of the gains we saw with respect to leaked windows.
status-firefox15:
--- → affected
tracking-firefox15:
--- → ?
Reporter | ||
Comment 24•12 years ago
|
||
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/
Assignee | ||
Comment 25•12 years ago
|
||
So have we decided that this is different from my stuff?
Assignee | ||
Comment 26•12 years ago
|
||
Ok, I bisected and this is different, but I'm still the one that broke it ;-) Patch coming up.
Assignee | ||
Comment 27•12 years ago
|
||
This re-removes code that was accidentally added back in in Bug 751999.
Attachment #630024 -
Flags: review?(jst)
Updated•12 years ago
|
Attachment #630024 -
Flags: review?(jst) → review+
Assignee | ||
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a7a905fd70d5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Assignee | ||
Comment 29•12 years ago
|
||
Comment on attachment 630024 [details] [diff] [review] Patch Virtually no risk, fixes leaks.
Attachment #630024 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #630024 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 30•12 years ago
|
||
khuey, can you land this on Aurora? :)
Comment 31•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/df5c5255c62c
status-firefox16:
--- → fixed
Assignee | ||
Comment 32•12 years ago
|
||
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.
Description
•