Closed Bug 424829 Opened 12 years ago Closed 12 years ago

Larry's IdentityHandler can hold windows alive for too long

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: bent.mozilla, Assigned: Gavin)

Details

(Keywords: memory-leak)

Attachments

(1 file, 1 obsolete file)

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
Flags: blocking-firefox3?
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.
Attached patch patch (obsolete) — Splinter Review
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).
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #311418 - Flags: review?(johnath)
(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 :)
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → Firefox 3
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.
Attachment #311418 - Flags: review?(johnath) → review+
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Whiteboard: [has patch][has reviews]
Attached patch with commentSplinter Review
Attachment #311418 - Attachment is obsolete: true
mozilla/browser/base/content/browser.js 	1.1014
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
You need to log in before you can comment on or make changes to this bug.