424829 - Larry's IdentityHandler can hold windows alive for too long

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Not really sure of STR, but I was trying to get to nytimes.com when our network went down (yay!). The load error page came up and the cycle collector began spewing lots of debug info that seems to implicate Larry. Here's the output:

nsCycleCollector: nsGlobalWindow 0x1bc864ec was not collected due to 2
  external references (4 total - 2 known)
  An object expected to be garbage could be reached from it by the path:
    nsGlobalWindow 0x1bc864ec
        via mInnerWindowHolders[i]
    XPCWrappedNative (ChromeWindow) 0x1bc8aeb0
        via <unknown edge>
    JS Object (ChromeWindow) (global=1bbfa8e0) 0x1bbfa8e0
        via gIdentityHandler
    JS Object (Object) (global=1bbfa8e0) 0x1c3efaa0
        via _lastLocation
    XPCNativeWrapper (Location) (global=1bf56780) 0x1c3efa40
        via wrappedNative.flatJSObject
    JS Object (Location) (global=1bf56780) 0x1c3ef9a0
        via __proto__
    JS Object (XPC_WN_NoMods_NoCall_Proto_JSClass - Location) (global=1bf56780) 0x1c3ef980
        via __proto__
    JS Object (Object) (global=1c3d61e0) 0x1c3d6200
        via __parent__
    JS Object (Window) (global=1c3d61e0) 0x1c3d61e0
        via <unknown edge>
    nsGlobalWindow 0x1c4c1f8c
  The 2 known references to it were from:
    XPCWrappedNative (ChromeWindow) 0x1bc87300
    JS Object (ChromeWindow) (global=1bbfa6a0) 0x1bbfa6a0

Comment 1

[:Gavin Sharp [email: [e-mail]]]

We hold a reference to the current document's nsLocation in _lastLocation, which is updated in onSecurityChange. We don't release it if onSecurityChange isn't called, which might be the case for an error page load.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

An easy way to fix this would be to pass in [location.host, location.hostname], since those are the only properties of the object we use.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Like so. This moves the property gets into the normal onSecurityChange path, when they used to only occur in the EV or popup-open cases - hopefully that won't be a perf problem. This does optimize the cases where we used to get the object off the nsLocation more than once (for EV with the "show domain/eTLD" pref set).

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> This does optimize the cases where we used to get the object off the nsLocation

"the properties off the nsLocation", I meant.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Though now that I look at the nsLocation::GetURI code (which is used by GetHostname and GetHost), it seems like just a roundabout way of getting to gBrowser.currentURI. Maybe we should just switch to using that.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Oh, but nsLocation::GetURI() it also gets the inner URI, and does fixup, which we need here - I think we might have gone through this before :)

Comment 7

[Johnathan Nightingale [:johnath]]

Comment on attachment 311418 [details] [diff] [review]
patch

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>-    getIdentityHandler().checkIdentity(this._state, gBrowser.contentWindow.location);
>+
>+    var location = gBrowser.contentWindow.location;
>+    var locationObj = {};
>+    try {
>+      locationObj.host = location.host;
>+      locationObj.hostname = location.hostname
>+    } catch (ex) {
>+      // Can sometimes throw if the URL being visited has no host/hostname,
>+      // e.g. about:blank. The _state for these pages means we won't need these
>+      // properties anyways, though.
>+    }
>+    getIdentityHandler().checkIdentity(this._state, locationObj);
>   },

Add a comment above this block explaining why we're doing this instead of just passing in location?  Something simple like:

// Don't pass in the actual location object, since it can cause us to 
// hold on to the window object too long.  Just pass in the fields we
// care about. (bug 424829)

With that change, looks good to me.  I don't think this needs tests beyond the leak tests that found it in the first place.  I do think this should block, since it's a leak fix with very low risk.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: with comment

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/browser.js 	1.1014