475283 - contentAreaUtils.js is polluting global namespace with getStringBundle(), replace it with ContentAreaUtils.stringBundle

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
function getStringBundle(bundleURI)
{
  try {
    Components.classes["@mozilla.org/intl/stringbundle;1"]
       .getService(Components.interfaces.nsIStringBundleService)
       .createBundle(bundleURI);
    return gStringBundleServiceCache.createBundle(bundleURI);
  } catch (e) {
    throw new Exception("Failed to get stringbundle URI <" + bundleURI + ">. Error: " + e);
  }
}

1. alert(getStringBundle("chrome://messenger/locale/accountCreationUtil.properties").GetStringFromName("cannot_contact_server.error"));
2. alert(getStringBundle("chrome://messenger/locale/accountCreationUtil.properties").GetStringFromName("DefaultSaveFileName"));

Actual result:
1: throws [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
2: returns "index" from toolkit/locales/en-US/chrome/global/contentAreaCommands.properties

Obviously, toolkit/content/contentAreaUtils.js line 681 is defining getStringBundle(), and polluting the global namespace (of anybody who includes it) with its private little string bundle.
contentAreaUtil.js was included in our window by some other developer.

If I rename my getStringBundle() to getStringBundle2(), it works (2 works, 1 fails, as expected).

Expected result:
Toolkit scripts don't use generically sounding names for things which are not generic and not for public use nor defined.

Severity: I just spent 2 hours trying to figure out why the string can't be found.

Fix:
Rename getStringBundle() in contentAreaUtil.js to getContentAreaUtilsStringBundle() (3 usages only).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What are you suggesting we do to fix this bug?

Any function name we choose is going to "pollute the global namespace", and changing to some more obscure name is likely to break more extensions than it fixes, given that this function is generally useful, and has had that name for at least 4 years and multiple Firefox releases.

I think this is WONTFIX.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Sorry, I missed your suggested fix.

I just did a search on http://mxr-test.konigsberg.mozilla.org/addons/ (index of the source of all AMO extensions) and it appears to not be a commonly called as I expected it to be (I couldn't find a caller that depended on the toolkit version).

I'm still wary of making changes like these (that index is old and represents only a subset of Firefox extensions), but perhaps we could try it if we do it early in a cycle (e.g. now, on trunk).

Comment 3

[Ben Bucksch (:BenB)]

Yes. I don't think it's very reasonable for an extension to rely on this function. Maybe they use the properties files, but most likely not the function, with this name. If they do, it's probably an accident like in my case.

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Patch, v1

Patch, v1

Untested so far. Need to update FF. Will report on test results.

Comment 5

[Ben Bucksch (:BenB)]

Comment on attachment 358880 [details] [diff] [review]
Patch, v1

I'm running with the patch since a while and haven't noticed any problems.

Comment 6

[Serge Gautherie (:sgautherie)]

Please, rather port bug 445831 ;-)

Comment 7

[:Paolo Amadini]

Ben, reducing global namespace pollution seems like a good idea. The patch for
bug 506116 already took a step in this direction, introducing this namespace:

+var ContentAreaUtils = {
+  get ioService() {
+    delete this.ioService;
+    return this.ioService =
+      Components.classes["@mozilla.org/network/io-service;1"]
+                .getService(Components.interfaces.nsIIOService);
+  }
+}

I think that reimplementing getStringBundle as a getter on the
ContentAreaUtils object would be great, maybe using the same caching
technique as above.

Since it is meant for internal use, I'd also prefix the getter name
with an underscore (something like "get _stringBundle()" or
"get _commandsStringBundle()").

Let's also hear Gavin's thoughts on this.

Comment 8

[:Paolo Amadini]

(In reply to comment #6)
> Please, rather port bug 445831 ;-)

While this may be appropriate for SeaMonkey, I advise against using this
technique in Toolkit, since it introduces a dependency on the XUL document
where the script is loaded, which would then need to define a specific
<stringbundle> element.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> I think that reimplementing getStringBundle as a getter on the
> ContentAreaUtils object would be great, maybe using the same caching
> technique as above.
> 
> Since it is meant for internal use, I'd also prefix the getter name
> with an underscore (something like "get _stringBundle()" or
> "get _commandsStringBundle()").

Makes sense to me.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 358880 [details] [diff] [review]
Patch, v1

Going to wait for a patch that addresses the comments in comment 7.

Comment 11

[:Paolo Amadini]

Using a "smart getter" like the one for ioService might also improve
performance slightly when the string bundle is needed.

Comment 12

[Ben Bucksch (:BenB)]

Paolo, feel free to take it over.

Comment 13

[:Paolo Amadini]

(In reply to comment #12)
> Paolo, feel free to take it over.

Thanks, I'm about to do other changes in the same file so it makes sense
that I do this small piece too.

Comment 14

[:Paolo Amadini]

Attachment: The change, with test case using file picker title keys

Comment 15

[Ben Bucksch (:BenB)]

Thanks, Paolo, for taking over!

Looking at the patch, it looks good, with 2 exceptions:
+  get _stringBundle() {
+    delete this._stringBundle;
+    return this._stringBundle = ....createBundle(...);
   }

1. don't delete it.
Rather, do:
 get _stringBundle() {
   if (!this.__stringBundle)
     this.__stringBundle = ....createBundle(...);
   return this.__stringBundle;
 }

2. You're using the same name for the getter and the internal properly variable. I'm not sure, but I think they should have separate names. That's why I above used 2x _ for the variable above.

+  delete ContentAreaUtils._stringBundle;
+  ContentAreaUtils._stringBundle = {
+    GetStringFromName: function() ""
+  };

You should implement formatStringFromName() as well.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> 1. don't delete it.
> Rather, do:
>  get _stringBundle() {
>    if (!this.__stringBundle)
>      this.__stringBundle = ....createBundle(...);
>    return this.__stringBundle;
>  }

Couldn't disagree more! :) The way the patch is now is the canonical way to implement memoizing getters - it avoids the function call overhead for subsequent accesses. It's fine as is.

(You could also use XPCOMUtil's defineLazyGetter, but no need to pull in that entire module for a single getter if it isn't already imported.)

Comment 17

[:Paolo Amadini]

(In reply to comment #15)
> +  delete ContentAreaUtils._stringBundle;
> +  ContentAreaUtils._stringBundle = {
> +    GetStringFromName: function() ""
> +  };
> 
> You should implement formatStringFromName() as well.

Well, I thought the same thing, but I saw that the approach in the rest of
this test file is to create mock objects that define just the functions that
are called in the tested code paths. nsIStringBundle contains four other
methods including formatStringFromName, adding stubs for each of them
doesn't hurt but I preferred to preserve the style of this part of the code.

Comment 18

[Dão Gottwald [:dao]]

A few extensions seem to use this to access the filesFolder, WebPageCompleteFilter and DefaultSaveFileName strings. Maybe this should remain public, just with a better name.

Comment 19

[:Paolo Amadini]

(In reply to comment #18)
> A few extensions seem to use this to access the filesFolder,
> WebPageCompleteFilter and DefaultSaveFileName strings.

The one I'm maintaining is one of them :-)

> Maybe this should remain public, just with a better name.

I'm not so sure this function was intended as a public API to begin with,
nor the contents of the string bundle itself. Extensions use them because
they have access to virtually anything, but authors know that they must
be prepared to update their code in response to changes like this, here
or in any other part of the code. As comment 2 suggests, if this change
is done on trunk we're probably safe enough, and to be safer we can
mention it in the release notes for extension developers.

I think the name "_stringBundle" for the getter on the ContentAreaUtils
object is as good as any other name, since in practice there's nothing
that stops extensions from accessing it; as I see it, the underscore in
the name is a hint that tells extension authors that they must be prepared
to see this function disappear without this being mentioned in the release
notes, and Toolkit developers that they can safely operate on the function
without wondering at each step if someone depends on its behavior or not.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 430734 [details] [diff] [review]
The change, with test case using file picker title keys

I don't really see any benefit to the underscore - we're unlikely to be changing this property again, and I don't think we really need to try and discourage its use (and the underscore probably isn't really an effective way to do that anyways). So r=me without it.

Comment 21

[:Paolo Amadini]

Ok, I'll generate a new patch where getStringBundle is a public getter (it's
true that it makes little difference). It's just that when I imagined an MDC
page documenting the public API of the ContentAreaUtils object and the rest
of the file (like the pages that exist for Places), getStringBundle seemed
out of place.

For other functions like internalPersist, are you fine with using the
underscore to mark them as private in case we move them inside the new
namespace in the future, even if extensions happen to use them?

Comment 22

[:Paolo Amadini]

Attachment: The change, with test case using file picker title keys

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2f20e1733c3d

Comment 24

[Ben Bucksch (:BenB)]

We gave birth!

(Thanks all :) )