339415 - In <builtinURLs.js>, |var SinkObserver| leaks once, as reported by Leak Monitor extension

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060526 SeaMonkey/1.5a

1. Install the Leak Detector Extension on SeaMonkey trunk.
2. Restart SeaMonkey
3. Open a new window (CTRL+1)
4. Close the original window.
5. Leak Detector window opens with the following report:

[+] [leaked object] (a58128) = [object Object]
 [+] onBeginLoad (a58120, chrome://communicator/content/builtinURLs.js, 12-14) = function (aSink) {
    gDataSourceState = (gDataSourceState | 1);
    debug_dump("\n-> SinkObserver:onBeginLoad: " + aSink + ", gDataSourceState=" + gDataSourceState + "\n");
}
  [ ] prototype (9c1540) = [object Object]
 [+] onInterrupt (a58050, chrome://communicator/content/builtinURLs.js, 17-19) = function (aSink) {
    gDataSourceState = (gDataSourceState | 2);
    debug_dump("\n-> SinkObserver:onInterrupt: " + aSink + ", gDataSourceState=" + gDataSourceState + "\n");
}
  [ ] prototype (9c15b8) = [object Object]
 [+] onResume (a58048, chrome://communicator/content/builtinURLs.js, 22-24) = function (aSink) {
    gDataSourceState = (gDataSourceState & -3);
    debug_dump("\n-> SinkObserver:onResume: " + aSink + ", gDataSourceState=" + gDataSourceState + "\n");
}
  [ ] prototype (9c1658) = [object Object]
 [+] onEndLoad (a58038, chrome://communicator/content/builtinURLs.js, 27-40) = function (aSink) {
    gDataSourceState = (gDataSourceState | 4);
    gDataSourceLoaded = (gDataSourceState == 5);
    debug_dump("\n-> onEndLoad: " + aSink + ", gDataSourceState=" + gDataSourceState + ", gDataSourceLoaded=" + gDataSourceLoaded + "\n");
    if (!gDataSourceLoaded) {
        debug_dump("\n-> builtin URLs not loaded!\n");
        return;
    }
    gBuiltinUrlsDataSource = aSink.QueryInterface(Components.interfaces.nsIRDFDataSource);
    debug_dump("Got gBuiltinUrlsDataSource " + gBuiltinUrlsDataSource + " with gTitleArc " + gTitleArc + " and gContentArc " + gContentArc + "\n");
}
  [ ] prototype (9c1670) = [object Object]
 [+] onError (a57c10, chrome://communicator/content/builtinURLs.js, 43-46) = function (aSink, aStatus, aErrMsg) {
    gDataSourceState = (gDataSourceState | 8);
    debug_dump("\n-> SinkObserver:onError: " + aSink + ", status=" + aStatus + ", errMsg=" + aErrMsg + ", gDataSourceState=" + gDataSourceState + "\n");
}
  [ ] prototype (9c1688) = [object Object]

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060517 SeaMonkey/1.1a] (nightly) (W98SE)

Confirmed with SMv1.1a branch.

This happens (only) "when closing the _first loaded_ window (either browser or jsconsole, depending on the command line I use)".

Comment 2

[Serge Gautherie (:sgautherie)]

Same leak if I startup [SMv1.1a] with "-mail" only.

Comment 3

[[e-mail]]

The leak only happens on the first window because once that particular data source has loaded it is no longer necessary to watch for it to load.

There seem to be at least three approaches to resolve this bug:
1. As documented in nsIRDFXMLSink.xml, the observer needs to be removed when it is no longer needed.
2. Instead of watching for the data source to load, check the load state manually each time we need the data source.
3. Instead of waiting for the data source to load, load the data source synchronously the first time we need it.

Comment 4

[Serge Gautherie (:sgautherie)]

I had a look at <http://developer.mozilla.org/en/docs/RDF_in_Mozilla_FAQ>:

3. "You should never do a synchronous load unless you really know what you're doing: this will freeze the UI until the load completes!"
Then, would it be acceptable in our case as the source is tiny and local ?
Would it have any (negative/positive) (startup) performance impact ? (little gain on startup, little loss on/if first usage !?)

1. "Note that the observer will remain attached to the RDF/XML datasource unless removeXMLSinkObserver is called."
No example :-< But adding |aSink.removeXMLSinkObserver(this);| works easily enough ;->

2. I would think one of the two other options are better...

Which has your preference ?

***

<http://lxr.mozilla.org/mozilla/search?string=builtinURLs.js>
{{
builtinURLs.js

/editor/ui/dialogs/content/EdSpellCheck.xul, line 55 -- <script type="application/x-javascript" src="chrome://communicator/content/builtinURLs.js"/>

/mail/base/content/utilityOverlay.xul, line 18 -- <script type="application/x-javascript" src="chrome://communicator/content/builtinURLs.js"/>
/mail/components/preferences/compose.xul, line 55 -- <script type="application/x-javascript" src="chrome://communicator/content/builtinURLs.js"/>
/mail/config/mail-jar.mn, line 41 -- content/communicator/builtinURLs.js, comm/content/communicator/builtinURLs.js

/xpfe/communicator/resources/content/utilityOverlay.xul, line 24 -- src="chrome://communicator/content/builtinURLs.js"/>
/xpfe/communicator/jar.mn, line 52 -- content/communicator/builtinURLs.js (resources/content/builtinURLs.js)
}}
(The loaders are few.)

<http://lxr.mozilla.org/mozilla/search?string=loadXURL>
{{
loadXURL
/xpfe/communicator/resources/content/builtinURLs.js, line 153 -- function loadXURL(key)
/xpfe/communicator/resources/content/builtinURLs.js, line 155 -- debug_dump("loadXURL call with " + key + "\n");
}}
This function seems to be dead code: can it be deleted !?

(Actually, that file calls for a few space and code improvements...)

*** (for the record)

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060524 SeaMonkey/1.1a] (nightly) (W98SE)

I activated the |function debug_dump(msg)| feature:
The .js file is loaded 1 time for each window (browser and jsconsole, in my case) during startup.

I can get all the SinkObserver messages doubled if I use "Pause" in the console after the "loadDS() called".

{{
-->loadDS() called for [object XULDocument] <--

-> SinkObserver:onBeginLoad: [xpconnect wrapped nsIRDFXMLSink], gDataSourceState=1

-> SinkObserver:onResume: [xpconnect wrapped nsIRDFXMLSink], gDataSourceState=1

-> SinkObserver:onInterrupt: [xpconnect wrapped nsIRDFXMLSink], gDataSourceState=3

-> SinkObserver:onResume: [xpconnect wrapped nsIRDFXMLSink], gDataSourceState=1

-> onEndLoad: [xpconnect wrapped nsIRDFXMLSink], gDataSourceState=5, gDataSourceLoaded=true
Got gBuiltinUrlsDataSource [xpconnect wrapped (nsISupports, nsIRDFXMLSink, nsIRDFDataSource)] with gTitleArc [xpconnect wrapped nsIRDFResource] and gContentArc [xpconnect wrapped nsIRDFResource]
}}

Comment 5

[Philip Chee]

> 1. "Note that the observer will remain attached to the RDF/XML datasource
> unless removeXMLSinkObserver is called."
> No example :-< But adding |aSink.removeXMLSinkObserver(this);| works
> easily enough ;->

> Which has your preference ?

I vote for [1]. Should this be done in the onEndLoad method? Don't you have to QI the interface first before invoking removeXMLSinkObserver?

Comment 6

[Serge Gautherie (:sgautherie)]

{{
	onEndLoad: function(aSink) {
		// Unhook observer.
		aSink.removeXMLSinkObserver(this);
}}
works fine ... You can find other examples with LXR.

Comment 7

[Philip Chee]

Attachment: Proposed patch v0.1

Something like this perhaps?

Comment 8

[Philip Chee]

Attachment: Proposed patch v0.2

Drat, wrong whitespace.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 223552 [details] [diff] [review]
Proposed patch v0.2

Yes, or preferably before the |if ... return;|.

I'm  waiting for neil's answers to prepare a patch with this and other fixes to this file...

Comment 10

[[e-mail]]

(In reply to comment #6)
>         onEndLoad: function(aSink) {
>                 // Unhook observer.
>                 aSink.removeXMLSinkObserver(this);
> works fine ... You can find other examples with LXR.
Looks good to me.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <pref-applications.js>

(Not this bug patch, but another file needed a similar fix.)

Unset the sink, |onError| too,
and space nits.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <builtinURLs.js>

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060524 SeaMonkey/1.1a] (nightly) (W98SE)

This adds the 2 missing |removeXMLSinkObserver()| calls,
and actually rewrite the wole file,
getting rid of what looked like a "tutorial" code (back at that time).

Comment 13

[[e-mail]]

(In reply to comment #12)
>Created an attachment (id=223574)
Well, you certainly cleaned up that file, but before I review I'd just like to know what jag thinks with particular reference to comment 3.

Comment 14

[[e-mail]]

Comment on attachment 223566 [details] [diff] [review]
(Bv1) <pref-applications.js>

Hmm... do we really want to abort this on error, such as might be caused by a new profile?

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> (From update of attachment 223566 [details] [diff] [review] [edit])
> Hmm... do we really want to abort this on error, such as might be caused by a
> new profile?

With regard to the code,
this is the only "other" file in the /mozilla tree that doesn't release the observer on error:
see <http://lxr.mozilla.org/mozilla/search?string=removeXMLSinkObserver>.
I understand/guess that onError is a terminal event (as onEndLoad is), is it not ?

With regard to the feature,
are you saying that we should do additional (which ?) things in the error case ?

Comment 16

[[e-mail]]

(In reply to comment #14)
>do we really want to abort this on error, such as might be caused by a new profile?
It turns out that this file is ensured to exist.

(In reply to comment #15)
>I understand/guess that onError is a terminal event (as onEndLoad is), is it not?
You always get an onEndLoad, unless of course you remove the observer earlier.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> (In reply to comment #14)
> >do we really want to abort this on error, such as might be caused by a new profile?
> It turns out that this file is ensured to exist.

... and is loaded from local/file:// not remote/http://,
contrary to the other sinks.

> (In reply to comment #15)
> >I understand/guess that onError is a terminal event (as onEndLoad is), is it not?
> You always get an onEndLoad, unless of course you remove the observer earlier.

Ah...
Then, I tested renaming the file or modifying its content: I get JS Errors, but onError is not called, always/only onEndLoad.

I read at
<http://lxr.mozilla.org/mozilla/source/rdf/base/idl/nsIRDFXMLSink.idl#56>
that "[onEndLoad is] Called when an RDF/XML load completes successfully.",
and I (wrongly) assumed onError would prevent that.
Anway, it would seem that this file will never hit the onError case.

Then, I shall not add removeXMLSinkObserver here,
but add a comment instead maybe !?

Comment 18

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060603 SeaMonkey/1.1a] (nightly) (W98SE)

(Bug still there, with L.M. v0.3.0.)

Comment 19

[[e-mail]]

Comment on attachment 223574 [details] [diff] [review]
(Av1) <builtinURLs.js>

We're not going to go this route, it's overkill for its purpose. We can simply use a stringbundle instead. Also all the new code belongs in mozilla/suite.

Comment 20

[Robert Kaiser]

In bug 328988, Pike seems to have found out the Thunderbird uses the xpfe builtinURLs.js through /mail/config/mail-jar.mn, line 41, and for retrieving one single URL from the toolkit builtinURLs.rdf - which might mean we should care not to break Thunderbird in the process.
The simple solution for that would be to move our builtinURLs.js to suite/ (which should happen anyways) and make Thunderbird use the unfixed one from xpfe/

Comment 21

[Robert Kaiser]

Investigation shows that the whole builtinURLS concept introduced by bug 35404 is obsolete and the handful of callers should be converted to normal stringbundle calls to some region.properties files.

See http://lxr.mozilla.org/mozilla/search?string=urn%3Aclienturl and http://lxr.mozilla.org/mozilla/search?string=xlateURL for the very few uses of that obsolete mechanism that have to be changed.

Comment 22

[[e-mail]]

Attachment: Just update builtinURLs.js to use a .properties file

This version avoids busting Thunderbird.

Comment 23

[[e-mail]]

Attachment: Move spellcheckers to editor region

Comment 24

[[e-mail]]

Attachment: Same idea but using a pref. [Checkin: Comment 32 & 35]

Comment 25

[Robert Kaiser]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]

This looks good to me for the SeaMonkey editor side, and it still seems to work :)

Comment 26

[[e-mail]]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]

seeking r= or moa=glazou

Comment 27

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]

r=me
Neil, please use [e-mail] for bugmail ; thanks

Comment 28

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060729 SeaMonkey/1.1a] (nightly) (W98SE)

Bug still there;
Waiting for "neil: superreview? (jag)"...

Comment 29

[[e-mail]]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]

I guess we still need to track down some consumers e.g. bug 337155 ;-)

Comment 30

[Robert Kaiser]

True, there is a new consumer in http://lxr.mozilla.org/mozilla/source/mailnews/compose/prefs/resources/content/pref-composing_messages.js#133 which has to be fixed the same way.

Additionally, we should probably apply the same fix to our other consumer of xlateURL(), which is at http://lxr.mozilla.org/mozilla/source/suite/common/pref/pref-locales.xul#163

See http://lxr.mozilla.org/mozilla/search?string=xlateURL and http://lxr.mozilla.org/mozilla/search?string=urn%3Aclienturl%3A for all current consumers - it looks to me that the urn:clienturl:toolbar: entries of builtinURLS.rdf are already unused.

Should this be all done in this bug or a new one be filed?

Once we've removed all users in our code, we also can remove the whole xlateURL()/builtinURLs implementation from suite/ - then Thunderbird will be the only consumers, and there's still their copy of the code in xpfe/communicator/ (which we aren't using any more).

Comment 31

[[e-mail]]

Attachment: pref-composing_messages [Checkin: Comment 36 & 38]

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]


Checkin: {
2006-07-30 03:15	neil%parkwaycc.co.uk
}

Comment 33

[Robert Kaiser]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]

Would be nice to obsolete builtinURLs on 1.8 branch as well :)

Comment 34

[Serge Gautherie (:sgautherie)]

Scott, you may want to port the following patch to Thunderbird,
so we could eventually get rid of this "builtinURLs" stuff on both apps:

(In reply to comment #24)
> Created an attachment (id=224438) [edit]
> Same idea but using a pref.

(In reply to comment #30)
> Once we've removed all users in our code, we also can remove the whole
> xlateURL()/builtinURLs implementation from suite/ - then Thunderbird will be
> the only consumers, and there's still their copy of the code in
> xpfe/communicator/ (which we aren't using any more).

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 224438 [details] [diff] [review]
Same idea but using a pref.
[Checkin: Comment 32 & 35]


Checkin: {
2006-07-31 13:27	neil%parkwaycc.co.uk 	MOZILLA_1_8_BRANCH
}

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 231293 [details] [diff] [review]
pref-composing_messages
[Checkin: Comment 36 & 38]


Checkin: {
2006-08-03 01:47	neil%parkwaycc.co.uk
}

Comment 37

[Robert Kaiser]

Comment on attachment 231293 [details] [diff] [review]
pref-composing_messages
[Checkin: Comment 36 & 38]

Yes, would be nice to have that on branch as well...

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 231293 [details] [diff] [review]
pref-composing_messages
[Checkin: Comment 36 & 38]


Checkin: {
2006-08-04 09:50	neil%parkwaycc.co.uk 	MOZILLA_1_8_BRANCH
}

Comment 39

[Scott MacGregor]

Hey guys, I've started to get rid of builtinURLS.rdf for Thunderbird as well after reading this bug report. That work is happening in: Bug 328988.

It is my understanding that Fx (and Tb) are going to use the following format for the add dictionaries URL:

http://addons.mozilla.org/$locale/thunderbird/$appversion/dictionaries/

See: https://bugzilla.mozilla.org/show_bug.cgi?id=343076#c17

I'm wondering if the seamonkey pref should be changed to use the same format? This pref would be non-localized.

If so, we could both share the same pref name and I could unfork EdSpellCheck.js if we can make SelectLanguage smart enough to do what it does today for seamonkey, while tossing the URL out to the OS for thunderbird. 

Comment 40

[Robert Kaiser]

mscott:
It may be a good idea to do that, and maybe to even unfork the file (though I'm not yet sure where the best border between forking and not forking is in the UI anyways).
Actually, I see no problem in using us the code you did in the patch there - as long as we don't have the "new" URL set, it just doesn't replace the values, if I see it correctly.
We want to use the URL wanted by AMO people, but I don't feel well by using URLs that don't work yet, as our nightly testers already complain about not finding some dictionaries in the working location we're pointing to atm...

Comment 41

[[e-mail]]

(In reply to comment #39)
>If so, we could both share the same pref name and I could unfork
>EdSpellCheck.js if we can make SelectLanguage smart enough to do what it does
>today for seamonkey, while tossing the URL out to the OS for thunderbird. 
Although I really think that there should be a core function somewhere that will usefully open a URL, how about this?

var ioService = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsIIOService);
uri = ioService.newURI(href, null, null);
var protocolSvc = Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
                            .getService(Components.interfaces.nsIExternalProtocolService);
if (protocolSvc.isExposedProtocol(uri.scheme))
  opener.open(href);
else
  protocolSvc.loadUrl(uri);

Comment 42

[Scott MacGregor]

I've posted a patch in Bug 328988 which unforks EdSpellCheck.js, uses the JS snippet Neil listed above for running the dictionary url. 

Currently I've left the seamonkey url in composer.js alone since that url actually works as opposed to the new format. Second, does seamonkey build have a XRE app info object which we use to extract the version when formatting the dictionary url? 

Comment 43

[Robert Kaiser]

Attachment: fix pref-locales to not use builtinURLs (checked in)

This patch fixes pref-locales to also go through a localized pref instead of  builtinURLs. With this, we should be ready to kill the whole builtinURLs mechanism in SeaMonkey.

I'm using a pref name that goes along with those used by Add-Ons, so that it probably can be reused there once Add-Ons adds a separate section for locale packs (along with switching, like for themes). Currently, it treats Language packs just like other extensions, but there are plans for the future to support them better from there.

Comment 44

[[e-mail]]

Comment on attachment 234600 [details] [diff] [review]
fix pref-locales to not use builtinURLs (checked in)

>-      openTopWin(xlateURL("urn:clienturl:viewmenu:intlwebcontent"));
>+      window.open(parent.hPrefWindow.getPref("localizedstring",
>+                                             "extensions.getMoreLocalesURL"));
Put this back to openTopWin, in case we get around to making it obey tabbed browsing preferences one day ;-)

Comment 45

[Robert Kaiser]

Comment on attachment 234600 [details] [diff] [review]
fix pref-locales to not use builtinURLs (checked in)

Checked in with Neil's nit fixed.

Now I think we just need to remove the builtinURLs mechanism from SeaMonkey code :)

Comment 46

[Robert Kaiser]

Attachment: remove builtinURLs functionality from SeaMonkey

This patch removes the builtinURLs functionality from SeaMonkey now that it's unused.
I'm not completely sure about the first hunk - is this file still used by Thunderbird and does it need that line? It might not, but I'm not 100% sure.
I've not remove the RDF itself as it sits in xpfe/ where we still have another copy of builtinURLs.js, but it's trivial to kill all that once Thunderbird has removed its use of the mechanism as well.

Comment 47

[Robert Kaiser]

(In reply to comment #42)
> I've posted a patch in Bug 328988 which unforks EdSpellCheck.js, uses the JS
> snippet Neil listed above for running the dictionary url. 

Nice, as you see here, we're also ready for killing builtinURLs completely now (actually, that mechanism is unused in SeaMonkey trunk as of today).

> Second, does seamonkey build have
> a XRE app info object which we use to extract the version when formatting the
> dictionary url? 

Yes, we do support it in all current SeaMonkey versions.

Comment 48

[[e-mail]]

Comment on attachment 234618 [details] [diff] [review]
remove builtinURLs functionality from SeaMonkey

>-  <script type="application/x-javascript" src="chrome://communicator/content/builtinURLs.js"/>
r+sr=me on the rest once Scott has handled this in bug 328988.

Comment 49

[Serge Gautherie (:sgautherie)]

Could the last two patches be ported to the 1.8 branch ?

Comment 50

[Robert Kaiser]

Serge:
Theoretically they could be ported, but we wouldn't approve them to be checked in. One reason for that is that it's too late in the game (L10n for 1.1 is about to be frozen within days), another that the last one isn't even checked in on trunk yet due to the dependency on a shared file (will be fixed on trunk in bug 328988).

Comment 51

[Robert Kaiser]

checked in the removal of the builtinURLS mechanism on trunk, so this should finally be completely fixed now.

Comment 52

[Simon Paquet [:sipaq]]

(In reply to comment #11)
> Created an attachment (id=223566) [details]
> (Bv1) <pref-applications.js>

Serge, your patch is now a year old and has never been reviewed. But the bug is marked FIXED for quite some time. Is the review of this patch still necessary or is your patch now obsolete?

Comment 53

[Robert Kaiser]

It probably belongs to a different bug, as noted when the patch was filed.
Serge, please file a new bug for that patch if it is still valid in current 2.0a1pre nightlies.

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 223566 [details] [diff] [review]
(Bv1) <pref-applications.js>

Given the later patches on this bug, and the fact that it is marked as fixed, this request seems no longer necessary. Therefore cancelling request. If the change is still required, please put the patch on a new bug and rerequest review.