896414 - Make nsDirectoryService::mHashtable an nsInterfaceHashtable

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 779142 [details] [diff] [review]
Patch v1

nsInterfaceHashtableMT is going away in Bug 849654.  Add your own locking around nsInterfaceHashtable if it's needed.

Comment 2

[Benjamin Smedberg]

The directory service isn't threadsafe to begin with, don't use a threadsafe hashtable here.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 4

[Benjamin Smedberg]

Comment on attachment 779226 [details] [diff] [review]
Patch v2

> NS_IMETHODIMP
> nsDirectoryService::Get(const char* prop, const nsIID & uuid, void* *result)
> {
>     NS_ENSURE_ARG(prop);
> 
>-    nsCStringKey key(prop);
>+    nsDependentCString key(prop);
>     
>-    nsCOMPtr<nsISupports> value = dont_AddRef(mHashtable.Get(&key));
>+    nsCOMPtr<nsIFile> cachedFile;
>+    mHashtable.Get(key, getter_AddRefs(cachedFile));

mHashtable.Get has an already_AddRefed version, use that instead.

> NS_IMETHODIMP
> nsDirectoryService::Set(const char* prop, nsISupports* value)
> {
>     NS_ENSURE_ARG(prop);
> 
>-    nsCStringKey key(prop);
>-    if (mHashtable.Exists(&key) || value == nullptr)
>+    nsDependentCString key(prop);
>+    if (mHashtable.Get(key, nullptr) || !value) {
>         return NS_ERROR_FAILURE;
>+    }
> 
>-    nsCOMPtr<nsIFile> ourFile;
>-    value->QueryInterface(NS_GET_IID(nsIFile), getter_AddRefs(ourFile));
>-    if (ourFile)
>-    {
>+    nsCOMPtr<nsIFile> ourFile = do_QueryInterface(value);
>+    if (ourFile) {
>       nsCOMPtr<nsIFile> cloneFile;
>       ourFile->Clone (getter_AddRefs (cloneFile));
>-      mHashtable.Put(&key, cloneFile);
>+      mHashtable.Put(key, cloneFile);
> 
>       return NS_OK;
>     }
> 
>     return NS_ERROR_FAILURE;   

If `value` doesn't QI to nsIFile, this method will silently do nothing. I know that's the prior behavior, but it's incorrect; please move `ourFile` and its QI to the top of this function and null-check that instead of `value`.

nit, please fix extraneous trailing whitespace while you're touching this function.

r=me with those changes

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 779226 [details] [diff] [review]
> Patch v2
> 
> > NS_IMETHODIMP
> > nsDirectoryService::Set(const char* prop, nsISupports* value)
> > {
> >     NS_ENSURE_ARG(prop);
> > 
> >-    nsCStringKey key(prop);
> >-    if (mHashtable.Exists(&key) || value == nullptr)
> >+    nsDependentCString key(prop);
> >+    if (mHashtable.Get(key, nullptr) || !value) {
> >         return NS_ERROR_FAILURE;
> >+    }
> > 
> >-    nsCOMPtr<nsIFile> ourFile;
> >-    value->QueryInterface(NS_GET_IID(nsIFile), getter_AddRefs(ourFile));
> >-    if (ourFile)
> >-    {
> >+    nsCOMPtr<nsIFile> ourFile = do_QueryInterface(value);
> >+    if (ourFile) {
> >       nsCOMPtr<nsIFile> cloneFile;
> >       ourFile->Clone (getter_AddRefs (cloneFile));
> >-      mHashtable.Put(&key, cloneFile);
> >+      mHashtable.Put(key, cloneFile);
> > 
> >       return NS_OK;
> >     }
> > 
> >     return NS_ERROR_FAILURE;   
> 
> If `value` doesn't QI to nsIFile, this method will silently do nothing. I
> know that's the prior behavior, but it's incorrect; please move `ourFile`
> and its QI to the top of this function and null-check that instead of
> `value`.

I don't follow. If ourFile ends up null, we skip the if and hit the "return NS_ERROR_FAILURE;" at the end of the function, no?

Comment 6

[Benjamin Smedberg]

Oh, indeed, I misread. It would still be nicer to rework it for an early return, but you can leave it this way if you want.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/710fe670c166