Last Comment Bug 778681 - Fix various incorrect nsresult usage
: Fix various incorrect nsresult usage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: nsresult-enum
  Show dependency treegraph
 
Reported: 2012-07-30 03:19 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-07-31 19:20 PDT (History)
1 user (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 -- Make some nsHTMLSelectElement methods infallible (3.07 KB, patch)
2012-07-30 04:16 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2 -- Convert nsXULDocument::mUnloadedOverlays from nsCOMArray to nsTArray (3.94 KB, patch)
2012-07-30 04:59 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray (3.60 KB, patch)
2012-07-30 04:59 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4 -- Convert nsCommandManager to use nsTArray instead of nsCOMArray (5.81 KB, patch)
2012-07-30 05:00 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review
Part 5 --Convert nsXULTemplateQueryProcessorRDF::mBindingDependencies from nsCOMArray to nsTArray (6.64 KB, patch)
2012-07-30 05:00 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review
Part 6 - Fix some misuses of bool as nsresult (2.68 KB, patch)
2012-07-30 05:01 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-07-30 03:19:40 PDT
(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 (away until August 15) 2012-07-30 03:22:34 PDT
(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 (away until August 15) 2012-07-30 04:16:36 PDT
Created attachment 647118 [details] [diff] [review]
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 (away until August 15) 2012-07-30 04:59:21 PDT
Created attachment 647126 [details] [diff] [review]
Part 2 -- Convert nsXULDocument::mUnloadedOverlays from nsCOMArray to nsTArray

Yay for fixing stuff properly!
Comment 4 :Aryeh Gregor (away until August 15) 2012-07-30 04:59:43 PDT
Created attachment 647127 [details] [diff] [review]
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray
Comment 5 :Aryeh Gregor (away until August 15) 2012-07-30 05:00:08 PDT
Created attachment 647128 [details] [diff] [review]
Part 4 -- Convert nsCommandManager to use nsTArray instead of nsCOMArray
Comment 6 :Aryeh Gregor (away until August 15) 2012-07-30 05:00:32 PDT
Created attachment 647129 [details] [diff] [review]
Part 5 --Convert nsXULTemplateQueryProcessorRDF::mBindingDependencies from nsCOMArray to nsTArray
Comment 7 :Aryeh Gregor (away until August 15) 2012-07-30 05:01:32 PDT
Created attachment 647131 [details] [diff] [review]
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 [:bz] 2012-07-30 08:09:22 PDT
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 [:bz] 2012-07-30 08:14:33 PDT
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 [:bz] 2012-07-30 08:15:23 PDT
Comment on attachment 647127 [details] [diff] [review]
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray

r=me
Comment 11 Boris Zbarsky [:bz] 2012-07-30 08:17:56 PDT
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 [:bz] 2012-07-30 08:20:43 PDT
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 [:bz] 2012-07-30 08:21:29 PDT
Comment on attachment 647131 [details] [diff] [review]
Part 6 - Fix some misuses of bool as nsresult

r=me

Note You need to log in before you can comment on or make changes to this bug.