The default bug view has changed. See this FAQ.

Fix various incorrect nsresult usage

RESOLVED FIXED in mozilla17

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(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. 
> ;)
(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.
Summary: content/, js/, layout/: Remove conversions to nsresult that will always result in NS_SUCCEEDED → Fix various incorrect nsresult usage
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.
Attachment #647118 - Flags: review?(bzbarsky)
Created attachment 647126 [details] [diff] [review]
Part 2 -- Convert nsXULDocument::mUnloadedOverlays from nsCOMArray to nsTArray

Yay for fixing stuff properly!
Attachment #647126 - Flags: review?(bzbarsky)
Created attachment 647127 [details] [diff] [review]
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray
Attachment #647127 - Flags: review?(bzbarsky)
Created attachment 647128 [details] [diff] [review]
Part 4 -- Convert nsCommandManager to use nsTArray instead of nsCOMArray
Attachment #647128 - Flags: review?(bzbarsky)
Created attachment 647129 [details] [diff] [review]
Part 5 --Convert nsXULTemplateQueryProcessorRDF::mBindingDependencies from nsCOMArray to nsTArray
Attachment #647129 - Flags: review?(bzbarsky)
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
Attachment #647131 - Flags: review?(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.
Attachment #647118 - Flags: review?(bzbarsky) → review+
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!
Attachment #647126 - Flags: review?(bzbarsky) → review+
Comment on attachment 647127 [details] [diff] [review]
Part 3 -- Convert nsXULTemplateQueryProcessorRDF::mQueries from nsCOMArray to nsTArray

r=me
Attachment #647127 - Flags: review?(bzbarsky) → review+
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.
Attachment #647128 - Flags: review?(bzbarsky) → review+
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.
Attachment #647129 - Flags: review?(bzbarsky) → review+
Comment on attachment 647131 [details] [diff] [review]
Part 6 - Fix some misuses of bool as nsresult

r=me
Attachment #647131 - Flags: review?(bzbarsky) → review+
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
Flags: in-testsuite-
Target Milestone: --- → mozilla17
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.