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

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: peterv)

Tracking

({mlk, testcase})

Trunk
mozilla10
x86_64
Linux
mlk, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7-)

Details

(Whiteboard: [MemShrink:P1][see comment 13])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 543693 [details]
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.
(Reporter)

Comment 1

6 years ago
Created attachment 543694 [details]
pageWithLink.html
(Reporter)

Updated

6 years ago
Version: 1.9.2 Branch → Trunk
(Reporter)

Comment 2

6 years ago
Filed bug 669097 for help finding out how many extensions are affected.
(Reporter)

Comment 3

6 years ago
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.....
tracking-firefox7: --- → ?

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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?
Whiteboard: [MemShrink]
(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

6 years ago
(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

6 years ago
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?
(Assignee)

Comment 12

6 years ago
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)]
(Assignee)

Comment 13

6 years ago
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.
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 14

6 years ago
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).
(Reporter)

Comment 19

6 years ago
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.
(Reporter)

Comment 21

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

Comment 22

6 years ago
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.
tracking-firefox7: ? → -

Updated

6 years ago
Whiteboard: [MemShrink:P1] → [MemShrink:P1][see comment 13]

Updated

6 years ago
Blocks: 669240
Blocks: 672619
(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.
(Assignee)

Comment 24

6 years ago
Created attachment 555801 [details] [diff] [review]
v1

I'll try to write an automated testcase.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #555801 - Flags: review?(mrbkap)
Could you use static_casts instead of C-style ones?
(Reporter)

Updated

6 years ago
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)
(Assignee)

Comment 26

6 years ago
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 #555801 - Flags: review?(mrbkap)
(Assignee)

Comment 28

6 years ago
Created attachment 567736 [details] [diff] [review]
v2
Attachment #555801 - Attachment is obsolete: true
Attachment #567736 - Flags: review?(mrbkap)

Updated

6 years ago
Attachment #567736 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 29

6 years ago
Created attachment 568199 [details] [diff] [review]
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.
Attachment #567736 - Attachment is obsolete: true
Attachment #568199 - Flags: review?(mrbkap)
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+
(Assignee)

Comment 31

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ed9d64a8d7
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/f7ed9d64a8d7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.