Closed Bug 896414 Opened 6 years ago Closed 6 years ago

Make nsDirectoryService::mHashtable an nsInterfaceHashtable

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #779142 - Flags: review?(benjamin)
Blocks: 464618
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.
Attachment #779142 - Flags: review-
The directory service isn't threadsafe to begin with, don't use a threadsafe hashtable here.
Attachment #779142 - Flags: review?(benjamin) → review-
Attached patch Patch v2Splinter Review
Attachment #779142 - Attachment is obsolete: true
Attachment #779226 - Flags: review?(benjamin)
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
Attachment #779226 - Flags: review?(benjamin) → review+
(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?
Summary: Make nsDirectoryService::mHashtable an nsInterfaceHashtableMT → Make nsDirectoryService::mHashtable an nsInterfaceHashtable
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.
https://hg.mozilla.org/mozilla-central/rev/710fe670c166
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.