Apparantly my changes to navigator.js are somehow causing a XUL document (the browser UI) to be leaked. We're trying to track down what it is, this is the tracker bug in the mean time.

So I've got two suspects which might be leaking, the rest of the checkin was just whitespace changes, style rewrites, etc. 1) _content.location.href changed to webNavigation.currentURI.spec 2) Assigning a string from a stringbundle to a member of XULBrowser: function nsXULBrowserWindow() { this.defaultStatus = bundle.GetStringFromName("defaultStatus"); }

And patch #2 is the winner...

So the offending bit is: getWebNavigation().currentURI; To rule out xbl, I tested this line, which didn't leak: getWebNavigation();

To clarify: this JS shouldn't leak... So we'll need to fix something somewhere else... Patch #2 is just a work-around we could use to bring current leak numbers down so smaller (new) leaks get noticed more easily in the mean time.

r=dbaron on that patch if you add something like "// XXX this causes a big leak and is temporarily comment out until we figure out why. See bug 61886" to both changes...

a=ben@netscape.com, I agree with dbaron, please add the suggested comment.

jag: have you figured out why you are leaking yet? ;-) (I just came across the comment when perusing navigator.js.) Gerv

Not really yet, just some hunches.

Does this still cause a leak?

Any ETA for the real thing yet? I really need this for MultiZilla.

dbaron is packing this weekend. adding some other folks that might be able to track this down. should this go on the 0.9.1 radar?

I just searched lxr on seamonkey for all usage of getWebNavigation().currentURI and everything I found was already commented out... from what I can tell all references of the offending line var url = getWebNavigation().currentURI.spec; has changed to var url = _content.location.href; which is what's suggested in patch #2, looks like work around patch has already checked in.

Mass move to jaggernaut@netscape.com

->moz1.0/helpwanted

Peter, is this Linux only, or is MS Windows also infected by this bug?

Well, currently no one is affected because of the work-arounds put in place. I don't recall who was and who wasn't affected, though I wouldn't be surprised if it's XP.

Well we are: http://MultiZilla.mozdev.org/index.html and we sure hope to see this fixed far before 1.0.

Are we sure this hasn't been fixed already? There was a JSPROP_SHARED that was missing somewhere in XBL stuff. I guess I'll try to test later today if the commented-out code is still around...

I vaguely remember this wasn't fixed with the JSPROP_SHARED fix you mention. Best way to find out is to go test.

Do we know that we'd still hit one of these running the bloat URLs?

We should hit that when the bookmarks "last visited" is updated.

There's no leaks anymore, so why can't we remove the workaround? I did, and have no problems with it. What is the holding it from going into the tree?

We could, or I could just remove the comments, it shouldn't matter really which version is used. Any strong preference from you, HJ?

Just a note. We apparently have security exploits that let a web page fake _content.location.href by setting the appropriate getters on things. So the nsIWebNavigation version is better in that it will definitely get the right URI.

-> 0.9.7

Thanks, I'll play with this once I remember how to do leak tests :-)

Comment on attachment 60110 [details] [diff] [review] Next update to trunk. r=dbaron

Fixed!

Jag, one small nit: @@ -123,7 +120,7 @@ function HandleBookmarkIcon(iconURL, addFlag) { - var url = getWebNavigation().currentURI.spec + var url = getWebNavigation().currentURI.spec; if (url) { // update URL with new icon reference if (!gBookmarksService)

I'll fix this next time I have to check in to navigator.js :-)