Closed Bug 933378 Opened 6 years ago Closed 3 years ago

Need to fix ErrorResult ergonomics in C++ code that declares one on the stack

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

27.94 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.38 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
3.47 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
ErrorResult is very hard to use properly from C++ code.  Consider this simple example:

  void foo() 
  {
    ErrorResult rv;
    DoSomeMethod(rv);
  }

What happens if DoSomeMethod() does a ThrowTypeError() on rv?  In debug builds that will fatally assert in ~ErrorResult and in opt build will leak the message.  The JS exception code on ErrorResult tries to deal with this by requiring possible callers of ThrowJSException to call MightThrowJSException() even if they're not throwing, so that even the non-throwing case asserts if the caller doesn't handle it...  But even then it's easy to screw up if the callee happens to not hit the codepath that might throw a JS exception during testing.

At the same time, we do not want to give ErrorResult per se a complicated non-trivial destructor.

Perhaps what we need is a class that inherits from ErrorResult (AutoErrorResult?) that does cleanup as needed?  I believe jib had to set up something link that for WebRTC code basically.  So the proposal would basically be:

1)  ErrorResult default ctor becomes protected.
2)  Non-binding consumers use AutoErrorResult which will at least unroot/delete
    properly or something.
3)  Binding consumers use detail::FastErrorResult which also inherits
    ErrorResult and does nothing in ctor/dtor.

Thoughts?
(In reply to Boris Zbarsky [:bz] from comment #0)
> Perhaps what we need is a class that inherits from ErrorResult
> (AutoErrorResult?) that does cleanup as needed?  I believe jib had to set up
> something link that for WebRTC code basically.

Yes, JSErrorResult - http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#103
Flags: needinfo?(peterv)
See Also: → 1224007
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(peterv)
Comment on attachment 8771123 [details] [diff] [review]
part 3.  Fix the ErrorResult usage in PeerConnectionImpl to not use ErrorResult directly anymore

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +141,5 @@
> +// This is a terrible hack.  The problem is that SuppressException is not
> +// inline, and we link this file without libxul in some cases (e.g. for our test
> +// setup).  So we can't use ErrorResult or IgnoredErrorResult because those call
> +// SuppressException...  And we can't use FastErrorResult because we can't
> +// include BindingUtils.h, because our linking is all fucked up.  Use

The convention in this file is s/all/completely/.

@@ +156,5 @@
>    }
> +
> +  operator ErrorResult&()
> +  {
> +    return *reinterpret_cast<ErrorResult*>(this);

I trust we'll never go virtual or use multiple inheritance in this class tree?
Attachment #8771123 - Flags: review?(jib) → review+
> The convention in this file is s/all/completely/.

Heh.  Will do.

> I trust we'll never go virtual or use multiple inheritance in this class tree?

I really hope not!

I suppose I could static_cast up to the parent class and then back down to this subclass if you prefer, though... That might be safer, at least as long as the subclasses never grow any virtual anything.
Flags: needinfo?(jib)
Good idea!
Flags: needinfo?(jib)
Attachment #8771125 - Attachment is obsolete: true
Attachment #8771125 - Flags: review?(bkelly)
Asked this in IRC, but repeating it here for those at home.

The setup in these patches is a bit unusual.  We're overlaying different class types on a base class simply to swap out the destructor.  It seems like we could be a little more explicit about our intention here by using a template functor.

Maybe something like this?

void
AssertAndSuppressCleanupFunc(ErrorResult* aRv)
{
  aRv->AssertReportedOrSuppressed();
  aRv->SuppressException();
}

template <typename CleanupFunc = AssertAndSuppressCleanupFunc>
class ErrorResult
{
  ~ErrorResult()
  {
    CleanupFunc(this);
  }
  
  ErrorResult<AssertAndSuppressCleanupFunc>& operator()
  {
    return *reinterpret_cast<ErrorResult<AssertAndSuppressCleanupFunc>*>(this);
  }
};

function SuppressCleanupFunc(ErrorResult* aRv)
{
  aRv->SuppressException();
}

typedef ErrorResult<SuppressCleanupFunc> IgnoredErrorResult;

function EmptyCleanupFunc(ErrorResult* aRv)
{
}

typedef ErrorResult<EmptyCleanupFunc> FastErrorResult;
Attachment #8771122 - Attachment is obsolete: true
Attachment #8771122 - Flags: review?(bkelly)
Attachment #8771213 - Attachment is obsolete: true
Attachment #8771213 - Flags: review?(bkelly)
Attachment #8771123 - Attachment is obsolete: true
Attachment #8771124 - Attachment is obsolete: true
Attachment #8771124 - Flags: review?(bkelly)
Comment on attachment 8771536 [details] [diff] [review]
part 1.  Introduce a TErrorResult class that will serve as a base class for various ErrorResult-like subclasses.  No actual behavior changes so far

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

::: dom/bindings/ErrorResult.h
@@ +18,5 @@
>   *    MaybeSetPendingException.
>   * 4) Converted to an exception JS::Value (probably to then reject a Promise
>   *    with) via dom::ToJSValue.
> + *
> + * An IgnoredErrorResult will automatically the first of those four things.

Will "automatically do the first"?
Attachment #8771536 - Flags: review?(bkelly) → review+
Attachment #8771537 - Flags: review?(bkelly) → review+
Comment on attachment 8771539 [details] [diff] [review]
part 4.  Change the ErrorResult destructor to suppress the exception, after asserting that it's already suppressed

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

Thanks!  I think this is a lot easier to understand than the previous version.  I appreciate you taking the time to indulge me.
Attachment #8771539 - Flags: review?(bkelly) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2d38b7c16b
part 1.  Introduce a TErrorResult class that will serve as a base class for various ErrorResult-like subclasses.  No actual behavior changes so far.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2bd3bdebd6
part 2.  Introduce a FastErrorResult class that bindings can use internally instead of ErrorResult.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33a9a88daa6
part 3.  Fix the ErrorResult usage in PeerConnectionImpl to not use ErrorResult directly anymore.  r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed38780a242c
part 4.  Change the ErrorResult destructor to suppress the exception, after asserting that it's already suppressed.  r=bkelly
Yeah, <sigh>.  IgnoredErrorResult used to be suppressed because it inherited from ErrorResult, which is explicitly whitelisted, but now it doesn't do that anymore.  Simple enough to add IgnoredErrorResult to the whitelist, though.
And same thing with FastErrorResult, of course.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2cbe0d4b29a
part 1.  Introduce a TErrorResult class that will serve as a base class for various ErrorResult-like subclasses.  No actual behavior changes so far.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3834c6ce6d7
part 2.  Introduce a FastErrorResult class that bindings can use internally instead of ErrorResult.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cdae42eaa89
part 3.  Fix the ErrorResult usage in PeerConnectionImpl to not use ErrorResult directly anymore.  r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9f5afda58e
part 4.  Change the ErrorResult destructor to suppress the exception, after asserting that it's already suppressed.  r=bkelly
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.