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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Ben, could you review the worker stuff?  Peter, if you want to do the rest?  Or delegate it to Ben if desired?  ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With the ErrorInfo.h file too

Comment 3

[Boris Zbarsky [:bzbarsky]]

roc suggests s/ErrorInfo/ErrorResult/, by the way.

For comparison, WebKit uses ExceptionCode, but they just have an integer there.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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

[Josh Matthews [:jdm]]

(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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, maybe I'll put it in mozilla.  Can't stick it int mfbt, but maybe that's ok.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and as far as comment 4 bracing nits, local style in that code is to not brace single-line if bodies.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Renamed to ErrorResult and with most of Ms2ger's nits fixed

Comment 11

[Peter Van der Beken [:peterv]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to Peter's comments

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1907bf7e6d7c

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be69a34372 for build bustage.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Weird.  It used to build!

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba9cc4ee095 with the extra needed include.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, it used to be ordered after another patch that indirectly added the include!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Masatoshi Kimura [:emk]]

(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.