778681 - Fix various incorrect nsresult usage

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #55)
> Comment on attachment 646561 [details] [diff] [review]
> content/, js/, layout/: Remove conversions to nsresult that will always
> result in NS_SUCCEEDED
> 
> I would prefer this actually do the error checks right.  Certainly in the
> query processor case.  And the XPConnect case, where I believe ignoring
> failure is a security bug.  And in the slider frame case, where it'll
> happily operate on uninitialized memory iirc...
> 
> Yes, that will change behavior.  Land it in a separate patch from this bug. 
> ;)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #57)
> Comment on attachment 646566 [details] [diff] [review]
> content/, dom/base/, embedding/, js/: Fix incorrect conversions to nsresult
> 
> The MorphSlimWrapper bit is wrong.  That really really needs to actually
> fail if it fails.
> 
> The script error reporting part looks kinda fishy too, but probably OK.
> 
> The rest are fine.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 -- Make some nsHTMLSelectElement methods infallible

mElements is an nsTArray and therefore infallible, so this shouldn't be returning anything to start with.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 -- Convert nsXULDocument::mUnloadedOverlays from nsCOMArray to nsTArray

Yay for fixing stuff properly!

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 -- Convert nsCommandManager to use nsTArray instead of nsCOMArray

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 --Convert nsXULTemplateQueryProcessorRDF::mBindingDependencies from nsCOMArray to nsTArray

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 6 - Fix some misuses of bool as nsresult

This is the only potentially nontrivial behavior change.  Try: https://tbpl.mozilla.org/?tree=Try&rev=82bafeda1920

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647118 [details] [diff] [review]
Part 1 -- Make some nsHTMLSelectElement methods infallible

r=me, especially if you document on the member that consumers depend on it being infallible.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647126 [details] [diff] [review]
Part 2 -- Convert nsXULDocument::mUnloadedOverlays from nsCOMArray to nsTArray

>+++ b/content/xul/document/src/nsXULDocument.cpp
>@@ -2580,21 +2581,20 @@ nsXULDocument::AddChromeOverlays()
>-    return rv;
>+    return NS_OK;

This looks wrong to me: if GetNext() returned an error, you just ate it.  Leave the rv there, please.

r=me with that, and thank you!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647127 [details] [diff] [review]
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647128 [details] [diff] [review]
Part 4 -- Convert nsCommandManager to use nsTArray instead of nsCOMArray

In AddCommandObserver, I don't think the nsAutoPtr is needed.  It's not like we have any early returns while it's in scope.

It might be nice to do:

  typedef nsTArray<nsCOMPtr<nsIObserver> > ObserverList;

or some such.

r=me with those fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647129 [details] [diff] [review]
Part 5 --Convert nsXULTemplateQueryProcessorRDF::mBindingDependencies from nsCOMArray to nsTArray

>+++ b/content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
> BindingDependenciesTraverser(nsISupports* key,
>     for (i = 0; i < count; ++i) {
>-        cb->NoteXPCOMChild(array->ObjectAt(i));
>+      cb->NoteXPCOMChild(array->ElementAt(i));

Local style is 4-space indent.

> nsXULTemplateQueryProcessorRDF::AddBindingDependency(nsXULTemplateResultRDF* aResult,
>     if (index == -1)
>-        return arr->AppendObject(aResult);
>+      arr->AppendElement(aResult);

4-space indent.

> nsXULTemplateQueryProcessorRDF::RemoveBindingDependency(nsXULTemplateResultRDF* aResult,
>         if (index >= 0)
>-            return arr->RemoveObjectAt(index);
>+          arr->RemoveElementAt(index);
>     }

And here.

Once again, a typedef might not be a terrible idea.

r=me with the nits.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647131 [details] [diff] [review]
Part 6 - Fix some misuses of bool as nsresult

r=me

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c984d97357
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2b27206ec4
https://hg.mozilla.org/integration/mozilla-inbound/rev/63ca1a86478a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07b4a92b5e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0231ccd54d
https://hg.mozilla.org/integration/mozilla-inbound/rev/17638d0b6c65

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91c984d97357
https://hg.mozilla.org/mozilla-central/rev/2c2b27206ec4
https://hg.mozilla.org/mozilla-central/rev/63ca1a86478a
https://hg.mozilla.org/mozilla-central/rev/e07b4a92b5e8
https://hg.mozilla.org/mozilla-central/rev/ff0231ccd54d
https://hg.mozilla.org/mozilla-central/rev/17638d0b6c65