470709 - [SeaMonkey] browser_bug388121-2.js leaks 1112 bytes

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081221 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b839ff0630c6
 +http://hg.mozilla.org/comm-central/rev/2a4c4c1f0feb + bug 469606 patch)

Bug 391318 comment 51 checkin "revealed" this (persisting) leak.
As a hint, bug 391318 comment 8 workaround does "fix" this leak (too)...

See also bug 450983 comment 39.

(Linking bug 406914 too, for now.)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: Windows leak log

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081221 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b839ff0630c6
 +http://hg.mozilla.org/comm-central/rev/2a4c4c1f0feb + bug 469606 patch)

Comment 2

[Serge Gautherie (:sgautherie)]

The leak is triggered by
|window.open("", "_blank", "width=10,height=10");|

though I can reproduce it with
|window.open("about:blank", "_blank", "width=10,height=10");|
if I immediately |finish(); return;| without running |setTimeout(testLoad, 10);|.

Comment 3

[Robert Kaiser]

We have a leak of the same size in other test suites than browser-chrome as well, by the way.

Our workaround from bug 391318 removed just 2 CompositeDataSourceImpl, 4 nsVoidArray and 2 XPCWrappedNative instances. What's still left is 22 objects summing up to 1112 bytes:

1 instance of BackstagePass with size 20 bytes
1 instance of CompositeDataSourceImpl with size 64 bytes
1 instance of InMemoryDataSource with size 144 bytes
1 instance of LocalStoreImpl with size 36 bytes
1 instance of RDFServiceImpl with size 272 bytes
1 instance of XPCNativeScriptableShared with size 108 bytes
4 instances of XPCWrappedNative with size 48 bytes each (192 bytes total)
2 instances of XPCWrappedNativeProto with size 32 bytes each (64 bytes total)
1 instance of nsGenericFactory with size 16 bytes
1 instance of nsJSID with size 32 bytes
1 instance of nsStringBuffer with size 8 bytes
1 instance of nsSystemPrincipal with size 32 bytes
3 instances of nsVoidArray with size 4 bytes each (12 bytes total)
1 instance of nsXPCWrappedJS with size 56 bytes
1 instance of nsXPCWrappedJSClass with size 40 bytes
1 instance of xptiInterfaceInfo with size 16 bytes 

If "rdf:extensions" is replaced by "rdf:null" in navigatorOverlay.xul, this drops to 0.
If the "Apply Themes" menupopup is reduced to |<menupopup datasources="rdf:extensions"></menupopup>| the leak still persists.
If I try to null out the .database property of that element on window shutdown, the leak persists.

The LocalStoreImpl appearing in the leak output sounds interesting somehow... Or might the leak be connected with the RDF cache somehow?

Comment 4

[Robert Kaiser]

If I set the menupop datasources attribute to "rdf:null" and add those lines in navigator.js top-level (i.e. window-global scope), I still get no leak:

var rdfService = Components.classes["@mozilla.org/rdf/rdf-service;1"]
                           .getService(Components.interfaces.nsIRDFService);
var extensionDS = rdfService.GetDataSource("rdf:extensions");

So it looks like we hold on to the datasources (both the EM component one and the localstore) only when XUL does refer to them and not when JS does.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> We have a leak of the same size in other test suites than browser-chrome as
> well, by the way.

These would be (bug 470710 and) bug 473686.

Your detailed findings seem consistent with what I found in comment 0 and bug 473686 comment 17.

(In reply to comment #4)
> So it looks like we hold on to the datasources (both the EM component one and
> the localstore) only when XUL does refer to them and not when JS does.

Interesting :-)

Comment 6

[[e-mail]]

Attachment: Add the datasource using JS

Comment 7

[Robert Kaiser]

Comment on attachment 364914 [details] [diff] [review]
Add the datasource using JS

>@@ -768,9 +768,7 @@
>   // remove the extension manager RDF datasource to prevent 'leaking' it
>   // see bug 391318, the real cause of reporting leaks is probably bug 406914
>   var extDB = document.getElementById('menu_ViewApplyTheme_Popup').database;
>-  var extDS = extDB.GetDataSources();
>-  while (extDS.hasMoreElements())
>-    extDB.RemoveDataSource(extDS.getNext());
>+  extDB.RemoveDataSource(gExtDS);

We should fix the comment here as well.

Apart from that, this patch seems to really fix the leaks!

Comment 8

[[e-mail]]

Sorry, I don't know how the tests work... is there any way to tell whether the code actually runs, or maybe it works because the code doesn't get run?

Comment 9

[[e-mail]]

Maybe Enn has an idea on why adding the datasource via the attribute leaks but via script does not.

Comment 10

[Robert Kaiser]

(In reply to comment #8)
> Sorry, I don't know how the tests work... is there any way to tell whether the
> code actually runs, or maybe it works because the code doesn't get run?

I can't tell the details of how the tests work, but I could verify that the two shipped themes are displayed in the menu in a browser window during the browser-chrome tests with your patch applied, so the datasource seems to be attached.

Comment 11

[[e-mail]]

The problem here is that we close the navigator before it's finished initialising and therefore the unload handler fails to clean up correctly.

Bug 243984 is another symptom of the underlying problem.

Comment 12

[[e-mail]]

Sorry, I mean bug 243894 of course...

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> The problem here is that we close the navigator before it's finished
> initialising and therefore the unload handler fails to clean up correctly.

Is this a problem with the test (which should use an event listener as browser_bug388121-1.js does?) or is SeaMonkey fragile and needs a fix?

Would you know why Firefox does not have this leak/bug ?

Comment 14

[[e-mail]]

(In reply to comment #13)
> Is this a problem with the test (which should use an event listener as
> browser_bug388121-1.js does?) or is SeaMonkey fragile and needs a fix?
Probably better to make SeaMonkey "safer", as per bug 243894.

> Would you know why Firefox does not have this leak/bug ?
Different bookmarks implementation.

Comment 15

[[e-mail]]

Attachment: Proposed patch

What I think is happening here is that when you open and close the window very quickly the controller doesn't exist yet and so can't be removed. However my build has too many other leaks for me to tell whether this helps, so hopefully KaiRo can verify its effect on leaks.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> hopefully KaiRo can verify its effect on leaks.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090410 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/1a8349d0cff8
 +http://hg.mozilla.org/comm-central/rev/671042296bda)

Ah, good point! :-)
This fixes bug 243894 exception which aborts the script before bug 391318 code runs...
Thus this fixes the 1112 leak seen in this bug and in bug 473686.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 373132 [details] [diff] [review]
Proposed patch

>-  controllers.removeController(BookmarksMenuController);

Maybe add a blank line here?
Also, could add a comment to explain why |try| is needed!

>+  try {

Comment 18

[jag (Peter Annema)]

Comment on attachment 373132 [details] [diff] [review]
Proposed patch

Yeah, a blank line and a comment would be good.

I'm not sure why I put the removeProgressListener(...) call in a try/catch, but it could probably use a similar comment, since it too will throw if we're trying to remove a listener that was never added.

I suspect I put it in a try/catch way back when because there were all these scary getInterface()s and QueryInterface()s going on and you never knew when those might fail!

Comment 19

[Robert Kaiser]

Comment on attachment 373132 [details] [diff] [review]
Proposed patch

Looks like that's the real cause for the workaround failing, the leaks go away with this here! r=me.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> I'm not sure why I put the removeProgressListener(...) call in a try/catch, but
> it could probably use a similar comment, since it too will throw if we're
> trying to remove a listener that was never added.

I don't know what the current situation is for that one:
if I remove the try+catch, at least this bug case doesn't throw there.

Anyway, I agree a comment of some sort is welcome.

Comment 21

[Robert Kaiser]

Windows doesn't come far enough due to other problems, but Linux seems to have no leaks on any tests now with this checkin.
Somehow, the Mac machine still reports the same 1112 byte leak on all tests, though.

Comment 23

[Robert Kaiser]

This looks fixed for Windows (just had a green cycle to confirm) and Linux, as bug 470710 is already filed on Mac-specific 1112-byte leaks, I suggest to mark the one here fixed and investigate/solve the Mac problem over in bug 470710.

Comment 24

[[e-mail]]

Pushed changeset 2f3bef40ea9e to comm-central.

Comment 25

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderboxes.

Comment 26

[Serge Gautherie (:sgautherie)]

KaiRo decreased the threshold:
http://hg.mozilla.org/build/buildbot-configs/rev/ac57604c3b64
fixed a lot of leaking, unfortunately OS X still has some left