125324 - Improper use of nsIDirectoryServiceProvider

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Conrad Carlen (not reading bugmail)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/filepicker/res/content/filepicker.js#133

  This code is getting a file location directly from a provider instead of
nsIDirectoryService. It happens to work because the object with the conract ID
"@mozilla.org/file/directory_service;1" happens to implement
nsIDirectoryServiceProvider as well as nsIDirectoryService and that provider
happens to provide the "Home" location. The service should not be QI'able to a
provider. It breaks the contract that when the service is asked for a file
location, registered providers are consulted.
  BTW, somebody just asked me why their js code using directory service wasn't
working. It was because they used this code as an example ;-)

The code should do something like this:

var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
             .getService( Components.interfaces.nsIProperties );
homeDir = dirSvc.get("Home", Components.interfaces.nsIFile);

Comment 1

[Brian Ryner (not reading)]

Please don't assign bugs to my old email address.

Comment 2

[timeless]

.

Comment 3

[timeless]

Attachment: this should work

Comment 4

[timeless]

Comment on attachment 108513 [details] [diff] [review]
this should work

i ran the changes through xpcshell on windows, but someone should test the
xpfilepicker itself :)

Comment 5

[Conrad Carlen (not reading bugmail)]

+  var dirServiceProvider = Components.classes[nsIDirectoryService_CONTRACTID]
+                                     .getService(nsIDirectoryService);

You want nsIProperties here, not nsIDirectoryService. The latter has no "get."
Also, I'd change the name of "dirServiceProvider" to "dirService."

   var persistent = new Object();
-  homeDir = dirServiceProvider.getFile("Home", persistent);
+  homeDir = dirServiceProvider.get("Home", Components.interfaces.nsIFile,
persistent);

Comment 6

[timeless]

Attachment: better

per ccarlen

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 108679 [details] [diff] [review]
better

homeDir is used elsewhere in this file; not initializing it is a bad idea.

Comment 8

[timeless]

Attachment: ok

Comment 9

[jag (Peter Annema)]

Comment on attachment 129315 [details] [diff] [review]
ok

>Index: mozilla/xpfe/components/filepicker/res/content/filepicker.js
>===================================================================

>+const nsIProperties       = Components.interfaces.nsIProperties;
>+const nsIDirectoryService_CONTRACTID = "@mozilla.org/file/directory_service;1";

This should really be |NS_DIRECTORYSERVICE_CONTRACTID|, or
|nsDirectoryService_CONTRACTID| to keep in this file's style. I think the only
accepted exceptions in our js code to "constants should be all uppercase" are
the interface consts which are used as shortcuts for Components.interfaces.*.

Comment 10

[Conrad Carlen (not reading bugmail)]

Comment on attachment 129315 [details] [diff] [review]
ok

thanks. r=ccarlen.

Comment 11

[timeless]

mozilla/xpfe/components/filepicker/res/content/filepicker.js	1.86

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED