669096 - Injecting function from chrome extends the lifetime of navigated-away inner window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: simple-injector.xpi

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.

Comment 1

[Jesse Ruderman]

Attachment: pageWithLink.html

Comment 2

[Jesse Ruderman]

Filed bug 669097 for help finding out how many extensions are affected.

Comment 3

[Jesse Ruderman]

This API was added in bug 549539, and I believe the example there is affected.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.....

Comment 5

[Andreas Gal :gal]

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.

Comment 6

[Jesse Ruderman]

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.

Comment 7

[Nicholas Nethercote [inactive]]

(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?

Comment 8

[Nicholas Nethercote [inactive]]

(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.

Comment 9

[Unknown W. Brackets]

(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]

Comment 10

[Unknown W. Brackets]

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]

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 12

[Peter Van der Beken [:peterv]]

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)]

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

[Asa Dotzler [:asa]]

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?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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...

Comment 17

[Justin Lebar (not reading bugmail)]

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.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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).

Comment 19

[Jesse Ruderman]

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.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 21

[Jesse Ruderman]

Can you not do that in this bug, please? :)

Comment 22

[Asa Dotzler [:asa]]

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.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: v1

I'll try to write an automated testcase.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you use static_casts instead of C-style ones?

Comment 26

[Peter Van der Beken [:peterv]]

Jesse: if you want a bug for porting InstallTrigger to nsIDOMGlobalPropertyInitializer you should file one (though it might be bug 669240).

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: v2.1

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 30

[Blake Kaplan (:mrbkap) (inactive)]

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++.

Comment 31

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ed9d64a8d7

Comment 32

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f7ed9d64a8d7