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

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.

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.

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

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

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.

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

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