Closed Bug 669096 Opened 10 years ago Closed 10 years ago
Injecting function from chrome extends the lifetime of navigated-away inner window
If chrome code injects a function into all pages, this prevents navigated-away inner windows from being collected. (I'm using the content-document-global-created pattern for injection; not sure if this matters.) 1. Install "Simple Injector" extension 2. Load pageWithLink.html 3. Click the link 4. Load about:memory in another tab 5. Click "minimize memory usage" Expected: the inner window for pageWithLink.html should go away. Result: the inner window remains. Closing the tab that contained pageWithLink.html allows the inner to be collected.
Filed bug 669097 for help finding out how many extensions are affected.
This API was added in bug 549539, and I believe the example there is affected.
Oh, I bet this is what jlebar is seeing.... In particular, I bet console.log hits this. If so, we should probably treat this as critical.....
Yeah, this sounds bad. Also, console.log is _incredibly_ slow. I get 200 messages a second or so in a tight loop and the browser freezes.
I suspect console.log is hitting a different bug. I don't encounter this problem if I use the nsIDOMGlobalPropertyInitializer pattern (and inject an object which functions hang off of) instead of the content-document-global-created pattern.
(In reply to comment #4) > Oh, I bet this is what jlebar is seeing.... You mean bug 668871 and/or bug 668809? jlebar, do you have any extensions installed?
(In reply to comment #7) > jlebar, do you have any extensions installed? Oh, bug 668871 comment 32 says he has Firebug, Adblock Plus, Bugzilla Tweaks and Telemetry.
(In reply to comment #4) I'm also experiencing the same issue he is, and I have Firebug and Adblock Plus installed. I don't have the other two he has (although I have others.) So it sounds like Firebug is indeed hitting this, probably with console.log. I probably can't go a day without Firebug, but I'll upgrade to Firebug 1.8.0b5 and disable the others, and report back if it continues to happen to me, in case that helps. -[Unknown]
Nightly 7.0a1 (2011-07-03) and Firebug 1.8.0b5. With only Firebug, I still get hanging compartments but it's not consistent. Calling console.log via content doesn't appear to do it even with Firebug showing these messages. I ran a series of tests using different features of Firebug and restarting in between each, and it seemed like using the Net tab in Firebug caused a compartment to stick. However, when trying to streamline a test against a well known site (www.google.com) for this, I couldn't reproduce it anymore. Using the as-standard Web Console net logging, inspecting, or the scratch pad, etc. works fine and the compartment seems to always go away. Having actually turned off Firebug for the first time in a while, I've come to realize this (the Web Console) is much nicer now than I thought. If I use a site like github, and traverse in and out of a folder (which uses popstate), it happens regardless of my extensions or other actions. It's fairly reproducible. This was mentioned in bug 668871 comment 16 and appears to be a separate issue. Facebook likeboxes also seem to stick around for a while/forever, although sometimes they appear to go away (eventually, *sometimes*.) This seems to happen regardless of extensions. Suspect a separate issue (probably not the same as popstate, but possibly?) So AFAICT it's not (directly) related to console.log, with or without Firebug. Not sure if that helps, my tests weren't nearly as conclusive as I was hoping for. -[Unknown]
> Also, console.log is _incredibly_ slow. File a bug, please? And make sure to test it on TM, since on m-c each call to console.log involves creating a new sandbox or something silly like that?
This doesn't reproduce consistently for me, it looks like I need to click fairly quickly on the link to get the leak. I also tried debugging in gdb and that made it disappear, so it might be timing related. I did find something weird in the CC log when it leaked: 0x7fffd05811a8 JS Object (Window) (global=7fffd05811a8) --[injectedFunction]--> 0x7fffd0590560 JS Object (Proxy) (global=7fffd0581088) --[parent]--> 0x7fffd0581088 [JS Object (Window) (global=7fffd0581088)] --[xpc_GetJSPrivate(obj)]-> 0x7fffcd1022b0 [XPCWrappedNative (Window)] --[mIdentity]-> 0x7fffce49c478 [nsGlobalWindow] The wrapper for the function set as a property on one inner window has as its parent a different inner window. Those inner windows both have the same outer window (see below) and afaict that parent is the old inner window. 0x7fffd0581088 JS Object (Window) (global=7fffd0581088) --[window]--> 0x7fffd0588028 [JS Object (Proxy) (global=7fffd05811a8)] --[window]--> 0x7fffd0588028 [JS Object (Proxy) (global=7fffd05811a8)]
Blake: when we create the new outer window we don't do anything about existing "waive" wrappers, so when we then get .wrappedJSObject of the new outer we end up with the existing waive wrapper which is parented to the old inner window. We find the old waive wrapper because the hash uses the JSObject* as the key, and we transplant the new outer in the old. If I reparent waive wrappers that were parented to the old inner window to be parented by the new inner window in nsGlobalWindow::SetNewDocument then the leak seems to go away.
bz, it's not clear to me what about this bug is "critical" or why you've nominated it for release driver tracking. Can you say specifically what it is you hope release drivers should do here? Is this something that breaks Firefox or breaks extensions?
(In reply to comment #14) > bz, it's not clear to me what about this bug is "critical" or why you've > nominated it for release driver tracking. Can you say specifically what it > is you hope release drivers should do here? Is this something that breaks > Firefox or breaks extensions? This bug may be causing lots of extensions to leak.
At the time I made my comment and nominated it was quite possible that any web page that used console.log() would effectively be leaked for the lifetime of the browser. It's not clear to me that this is not the case. Needs checking. It _is_ clear that the access patterns that Firebug uses seem to leak, which is something that we should probably care about. That said, I'm not sure there's much release drivers can do here until there's a patch, if this is not a regression...
At the last memshrink meeting, we decided to file a separate bug on Firebug (as separate from the testcase in this bug) and then dupe if they turn out to be the same issue. Filed bug 669730.
(In reply to comment #16) > At the time I made my comment and nominated it was quite possible that any > web page that used console.log() would effectively be leaked for the > lifetime of the browser. > > It's not clear to me that this is not the case. Needs checking. Pages are not leaking for the lifetime of the browser, or even the tab. They do appear to be staying around slightly longer than desirable (it takes two clicks of "minimize memory usage" after navigating away before the compartment dies).
Kyle, are you referring to my testcase or something involving console.log()? With my testcase the leak seemed to persist for the lifetime of the tab.
(In reply to comment #19) > Kyle, are you referring to my testcase or something involving console.log()? > With my testcase the leak seemed to persist for the lifetime of the tab. Sorry, should have made clear I was referring to console.log.
Can you not do that in this bug, please? :)
Release team isn't going to track this but if someone has a fix or more information that would cause us to want to track this, please pipe up.
Whiteboard: [MemShrink:P1] → [MemShrink:P1][see comment 13]
(In reply to Peter Van der Beken [:peterv] from comment #13) > [...] If I > reparent waive wrappers that were parented to the old inner window to be > parented by the new inner window in nsGlobalWindow::SetNewDocument then the > leak seems to go away. Peter, want to attach a patch that does this? It seems like the "best" way to fix this bug until compartment-per-global.
I'll try to write an automated testcase.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Could you use static_casts instead of C-style ones?
Summary: Injecting function from chrome extends the lifetime of navigated-away inner windows → Injecting function from chrome extends the lifetime of navigated-away inner windows (port InstallTrigger to nsIDOMGlobalPropertyInitializer)
Jesse: if you want a bug for porting InstallTrigger to nsIDOMGlobalPropertyInitializer you should file one (though it might be bug 669240).
Summary: Injecting function from chrome extends the lifetime of navigated-away inner windows (port InstallTrigger to nsIDOMGlobalPropertyInitializer) → Injecting function from chrome extends the lifetime of navigated-away inner window
Comment on attachment 555801 [details] [diff] [review] v1 peterv and I talked through this and there is a case where this patch won't fix the bug (namely: navigating cross origin and then back to the original origin). He has a plan to fix it.
Attachment #567736 - Flags: review?(mrbkap) → review+
The previous patch causes orange, because we can end up with an inner with no outer as the parent of a waive wrapper. I think not doing anything in that case should be fine, so this patch does that. It also clears any new exceptions that outerizing throws. Sorry for the double review, I didn't realize this could happen.
Comment on attachment 568199 [details] [diff] [review] v2.1 I think you can assert that there's no exception pending when we start the reparenting process, eliminating the need for mWasExceptionPending in the closure. Other than that, this looks good. It'll be really nice when the inner and outer window "hooks" are simply pointers to the inner our outer windows instead of having to re-compute them in C++.
Attachment #568199 - Flags: review?(mrbkap) → review+
Target Milestone: --- → mozilla10
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.