The default bug view has changed. See this FAQ.

Switch Paris bindings to using a struct for the result of fallible methods instead of a raw nsresult

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Patch coming up.  Longer-term, we can use this to pass out exception strings or whatnot, not just nsresults!

This applies on top of the patch in bug 745897.
Created attachment 618901 [details] [diff] [review]
Patch

Ben, could you review the worker stuff?  Peter, if you want to do the rest?  Or delegate it to Ben if desired?  ;)
Attachment #618901 - Flags: review?(peterv)
Attachment #618901 - Flags: review?(bent.mozilla)
Depends on: 745897
Created attachment 618907 [details] [diff] [review]
With the ErrorInfo.h file too
Attachment #618907 - Flags: review?(peterv)
Attachment #618907 - Flags: review?(bent.mozilla)
Attachment #618901 - Attachment is obsolete: true
Attachment #618901 - Flags: review?(peterv)
Attachment #618901 - Flags: review?(bent.mozilla)
roc suggests s/ErrorInfo/ErrorResult/, by the way.

For comparison, WebKit uses ExceptionCode, but they just have an integer there.
Comment on attachment 618907 [details] [diff] [review]
With the ErrorInfo.h file too

Review of attachment 618907 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2522,4 @@
>  {
>    char *data = static_cast<char*>(NS_Alloc(aBody.Length() + 1));
>    if (!data) {
> +    return aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

NS_Alloc is infallible...

::: content/canvas/src/WebGLContext.cpp
@@ +790,3 @@
>      JSObject* obj = GetContextAttributes(rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

::: content/canvas/src/WebGLContextGL.cpp
@@ +2743,4 @@
>      JS::Value v =
>          GetFramebufferAttachmentParameter(cx, target, attachment, pname, rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

@@ +3413,3 @@
>      JS::Value v = GetVertexAttrib(cx, index, pname, rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

::: dom/bindings/ErrorInfo.h
@@ +10,5 @@
> +
> +#ifndef mozilla_dom_ErrorInfo_h
> +#define mozilla_dom_ErrorInfo_h
> +
> +class ErrorInfo {

This should be in a namespace

@@ +17,5 @@
> +    mResult = NS_OK;
> +  }
> +
> +  void Throw(nsresult rv) {
> +    mResult = rv;

Assert NS_FAILED?

@@ +40,5 @@
> +    return mResult;
> +  }
> +
> +private:
> +  nsresult mResult;

You need to include nscore.h.
> NS_Alloc is infallible...

Who the hell did that?  And why did they not change this code in the process?  This MUST be a fallible allocation.

> This should be in a namespace

Er, yes.  Assume it's in mozilla::dom.  Though that _does_ make using it in headers a huge pain. :(

> Assert NS_FAILED?

I was considering that, yes.  Wasn't sure whether it was desirable in practice...

> You need to include nscore.h.

Yes.  Will do.

Comment 6

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #5)
> > NS_Alloc is infallible...
> 
> Who the hell did that?  And why did they not change this code in the
> process?  This MUST be a fallible allocation.

Bug 680556.
(In reply to Boris Zbarsky (:bz) from comment #5)
> Er, yes.  Assume it's in mozilla::dom.  Though that _does_ make using it in
> headers a huge pain. :(

You could put it in the mozilla namespace, and/or add a typedef to bring it into the scope of specific classes.
Yeah, maybe I'll put it in mozilla.  Can't stick it int mfbt, but maybe that's ok.
Oh, and as far as comment 4 bracing nits, local style in that code is to not brace single-line if bodies.
Created attachment 619575 [details] [diff] [review]
Renamed to ErrorResult and with most of Ms2ger's nits fixed
Attachment #619575 - Flags: review?(peterv)
Attachment #619575 - Flags: review?(bent.mozilla)
Attachment #618907 - Attachment is obsolete: true
Attachment #618907 - Flags: review?(peterv)
Attachment #618907 - Flags: review?(bent.mozilla)
Whiteboard: [need review]
Comment on attachment 619575 [details] [diff] [review]
Renamed to ErrorResult and with most of Ms2ger's nits fixed

Review of attachment 619575 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +977,4 @@
>      nsCOMPtr<nsICharsetConverterManager> ccm =
> +      do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
> +    if (NS_FAILED(rv)) {
> +      return aRv.Throw(rv);

This looks a bit odd. I think I'd prefer:

    aRv.Throw(rv);
    return;

::: dom/workers/XMLHttpRequestEventTarget.h
@@ +28,5 @@
>    _Finalize(JSFreeOp* aFop) MOZ_OVERRIDE;
>  
>  #define IMPL_GETTER_AND_SETTER(_type)                                          \
>    JSObject*                                                                    \
> +  GetOn##_type(ErrorResult& aRv)                                                 \

Please correct the whitespace.

@@ +34,5 @@
>      return GetEventListener(NS_LITERAL_STRING(#_type), aRv);                   \
>    }                                                                            \
>                                                                                 \
>    void                                                                         \
> +  SetOn##_type(JSObject* aListener, ErrorResult& aRv)                            \

And here.
Attachment #619575 - Flags: review?(peterv) → review+
> I think I'd prefer

OK.  I'll leave that pattern as-is in the canvas code, since the canvas folks prefer it, undo it in the DOM code.

Thanks for the review!
Created attachment 621087 [details] [diff] [review]
Updated to Peter's comments
Attachment #621087 - Flags: review?(bent.mozilla)
Attachment #619575 - Attachment is obsolete: true
Attachment #619575 - Flags: review?(bent.mozilla)
Comment on attachment 621087 [details] [diff] [review]
Updated to Peter's comments

Review of attachment 621087 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why we allow the operator= on ErrorResult. Shouldn't we just have Reset() or something to set to NS_OK?

::: dom/workers/EventListenerManager.cpp
@@ +283,5 @@
>  {
>    using namespace mozilla::dom::workers::events;
>  
>    if (!IsSupportedEventClass(aEvent)) {
>      aRv = NS_ERROR_FAILURE;

Er, shouldn't this be Throw() too?

::: dom/workers/EventTarget.cpp
@@ +44,5 @@
>  
>    JSString* type =
>      JS_NewUCStringCopyN(cx, aType.BeginReading(), aType.Length());
>    if (!type || !(type = JS_InternJSString(cx, type))) {
>      aRv = NS_ERROR_OUT_OF_MEMORY;

Throw?

::: dom/workers/XMLHttpRequest.h
@@ +79,5 @@
>    Notify(JSContext* aCx, Status aStatus) MOZ_OVERRIDE;
>  
>  #define IMPL_GETTER_AND_SETTER(_type)                                          \
>    JSObject*                                                                    \
> +  GetOn##_type(ErrorResult& aRv)                                                 \

Can you fix the \ alignment?
Attachment #621087 - Flags: review?(bent.mozilla) → review+
> I don't understand why we allow the operator= on ErrorResult.

Because there's a bunch of code that was doing:

  aRv = SomethingThatMightReturnError();

and I was trying to keep the size of the patch down.  We would have had to convert each of those to a local rv plus a branch...  I'm fine with doing that in followups; I agree that we should work on getting rid of the operator=.

> Er, shouldn't this be Throw() too?

Yes.  Will fix.  ;)

> Throw?

Yes.  Will fix.

> Can you fix the \ alignment?

Yes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1907bf7e6d7c
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla15
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be69a34372 for build bustage.
Weird.  It used to build!

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba9cc4ee095 with the extra needed include.
Ah, it used to be ordered after another patch that indirectly added the include!
Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/1907bf7e6d7c
https://hg.mozilla.org/mozilla-central/rev/b7be69a34372

Re-landed (and stuck):
https://hg.mozilla.org/mozilla-central/rev/4ba9cc4ee095
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #6)
> (In reply to Boris Zbarsky (:bz) from comment #5)
> > > NS_Alloc is infallible...
> > Who the hell did that?  And why did they not change this code in the
> > process?  This MUST be a fallible allocation.
> Bug 680556.
And I filed bug 705035.
You need to log in before you can comment on or make changes to this bug.