Closed Bug 1213289 Opened 4 years ago Closed 4 years ago

Add a way to throw a DOMException with a custom message on an ErrorResult

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This came up recently in both bug 1213077 and in some stuff Cameron was working on.  We should just fix it.
Status: NEW → ASSIGNED
Comment on attachment 8672006 [details] [diff] [review]
part 1.  Change dom::Throw to take an XPCOM string, not a raw C string, for the message

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

::: dom/bindings/Exceptions.cpp
@@ +133,5 @@
>    return false;
>  }
>  
>  void
> +ThrowAndReport(nsPIDOMWindow* aWindow, nsresult aRv)

I guess this change is simply due to no one calling it with a message any more?  So a bit orthogonal to the other changes here.

@@ +166,5 @@
>      break;
>    }
>  
>    // If not, use the default.
>    // aMessage can be null, so we can't use nsDependentCString on it.

Is this comment still right?  It seems aMessage cannot be nullptr now.
Attachment #8672006 - Flags: review?(bkelly) → review+
> I guess this change is simply due to no one calling it with a message any more?

Yes.  It's not entirely orthogonal, because if I didn't just nix the message argument here I'd need to change its type or put in an nsDependentCString or something, since the callee of ThrowAndReport is Throw and I'm changing its signature.

> Is this comment still right?

Good catch.  No.  Removed that line.
Comment on attachment 8672007 [details] [diff] [review]
part 2.  Introduce ErrorResult::ClearUnionData and use it in various places where we're trying to do that

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

r=me with comments addressed.

::: dom/bindings/BindingUtils.cpp
@@ -170,5 @@
>  ErrorResult::CreateErrorMessageHelper(const dom::ErrNum errorNumber, nsresult errorType)
>  {
> -  if (IsErrorWithMessage()) {
> -    delete mMessage;
> -  }

If we're removing this, maybe MOZ_ASSERT(!mMessage) to make sure we don't leak here in the future?

::: dom/bindings/ErrorResult.h
@@ +96,5 @@
>    ErrorResult(ErrorResult&& aRHS)
>    {
> +    // Initialize mResult so the ClearUnionData call in our operator= will do
> +    // the right thing (nothing).
> +    mResult = NS_OK;

I think it would be safer to call ErrorResult() in the constructor initializer list.
Attachment #8672007 - Flags: review?(bkelly) → review+
Comment on attachment 8672008 [details] [diff] [review]
part 3.  Add a way to throw a DOMException with a custom message on ErrorResult

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

r=me with comments addressed.  Did you test this with anything?

::: dom/bindings/BindingUtils.cpp
@@ +299,5 @@
>    js::RemoveRawValueRoot(cx, &mJSException);
>    mResult = NS_OK;
>  }
>  
> +struct ErrorResult::DOMExceptionInfo {

nit: It would be nice to define a default constructor that initializes mRv to something sane.  Also, a constructor taking the two values would make the code in ThrowDOMException() a little clearer.

@@ +324,5 @@
> +      !ReadParam(aMsg, aIter, &readInfo->mRv)) {
> +    return false;
> +  }
> +
> +  MOZ_ASSERT(!mHasDOMExceptionInfo);

Don't we need to assert the other union booleans aren't set either?  We wouldn't want to overwrite a message, etc.

@@ +360,5 @@
> +
> +void
> +ErrorResult::ClearDOMExceptionInfo()
> +{
> +  MOZ_ASSERT(IsDOMException());

I think MOZ_ASSERT(mHasDOMExceptionInfo) would be worth it here to help guard against enum confusion.  Deleting the union as the wrong type is arguably worse than reporting the wrong thing.

::: dom/bindings/ErrorResult.h
@@ +81,5 @@
>  
>  #ifdef DEBUG
>      mMightHaveUnreportedJSException = false;
>      mHasMessage = false;
> +    mHasDOMExceptionInfo = false;

nit: Why are the values in this constructor not set via the initializer list?

@@ +294,5 @@
>    // ReportErrorWithMessage.
>    // mJSException is set (and rooted) by ThrowJSException and unrooted
>    // by ReportJSException.
> +  // mDOMExceptionInfo is set by ThrowDOMException and cleared
> +  // (and deallocated) by ReportDOMException.

nit: Shouldn't this say cleared by ClearDOMException()?  Which is called by both ReportDOMException() and ClearUnionData().
Attachment #8672008 - Flags: review?(bkelly) → review+
Comment on attachment 8672008 [details] [diff] [review]
part 3.  Add a way to throw a DOMException with a custom message on ErrorResult

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

::: dom/bindings/ErrorResult.h
@@ +313,5 @@
> +  // Used to keep track of whether mDOMExceptionInfo has ever been assigned
> +  // to.  We need to check this in order to ensure that not attempting to
> +  // delete mDOMExceptionInfo in DeserializeDOMExceptionInfo doesn't leak
> +  // memory.
> +  bool mHasDOMExceptionInfo;

Actually, at this point, maybe we should just have a single enum value instead of three mutually exclusive booleans.
> If we're removing this, maybe MOZ_ASSERT(!mMessage)

I don't think we can; at this point mMessage might not even be initialized yet.  The only caller does ClearUnionData(), though...  I could duplicate the IsErrorWithMessage() check and then assert !mMessage, but that wouldn't catch the other cases (JSException/DOMException) anyway.

> I think it would be safer to call ErrorResult() in the constructor initializer list.

Oh, right, we can do that now.  Will do.

> Did you test this with anything?

I hand-modified querySelector to call ThrowDOMException and verified that my custom message came through (this did catch some issues in an earlier patch version).  I did a try run in general too.

> It would be nice to define a default constructor that initializes mRv to something sane.

I'm not sure there's anything really sane to initialize to.  But...

> Also, a constructor taking the two values

Totally excellent call.  Done.  This makes DeserializeDOMException a bit clearer too.  I don't think we need a default ctor given that.

> Don't we need to assert the other union booleans aren't set either? 

We should probably assert that IsDOMException(), in which case those other booleans better not be set.... Did that.

> I think MOZ_ASSERT(mHasDOMExceptionInfo) would be worth it here

OK, done, but there are in fact cases in which we have IsDOMException() but !mHasDOMExceptionInfo (e.g. right after calling ClearDOMExceptionInfo()).  So I made this assert be:

  MOZ_ASSERT(mHasDOMExceptionInfo || !mDOMExceptionInfo);

> nit: Why are the values in this constructor not set via the initializer list?

No clue.  Fixed.

> nit: Shouldn't this say cleared by ClearDOMException()? 

Hmm.  So in normal operation the only real caller of ClearDOMException should in fact be ReportDOMException (as in, people should not be clobbering exceptions).

I'll s/cleared/reported/ to make that clearer (reportier!).
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.