Closed
Bug 933378
Opened 11 years ago
Closed 8 years ago
Need to fix ErrorResult ergonomics in C++ code that declares one on the stack
Categories
(Core :: DOM: Core & HTML, defect)
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?
Assignee | ||
Updated•11 years ago
|
Blocks: ParisBindings
Comment 1•11 years ago
|
||
(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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(peterv)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8771122 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8771123 -
Flags: review?(jib)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8771124 -
Flags: review?(bkelly)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8771125 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(peterv)
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
> 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)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8771213 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8771125 -
Attachment is obsolete: true
Attachment #8771125 -
Flags: review?(bkelly)
Comment 10•8 years ago
|
||
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;
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8771536 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8771122 -
Attachment is obsolete: true
Attachment #8771122 -
Flags: review?(bkelly)
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8771537 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8771213 -
Attachment is obsolete: true
Attachment #8771213 -
Flags: review?(bkelly)
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8771123 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8771539 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Attachment #8771124 -
Attachment is obsolete: true
Attachment #8771124 -
Flags: review?(bkelly)
Comment 15•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8771537 -
Flags: review?(bkelly) → review+
Comment 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d12f1688d4710d9e31f32b1fcf360539655222c5 for hazard failures from one of this csets
Comment 19•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a795d0003a44 Backed out changeset ed38780a242c https://hg.mozilla.org/integration/mozilla-inbound/rev/4a25ec78baeb Backed out changeset b33a9a88daa6 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d50699deac1 Backed out changeset 1c2bd3bdebd6 https://hg.mozilla.org/integration/mozilla-inbound/rev/c907a44b0c6c Backed out changeset dd2d38b7c16b
Assignee | ||
Comment 20•8 years ago
|
||
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.
Assignee | ||
Comment 21•8 years ago
|
||
And same thing with FastErrorResult, of course.
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aa7b5dfa95 just to make sure I got the annotations right.
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d2cbe0d4b29a https://hg.mozilla.org/mozilla-central/rev/a3834c6ce6d7 https://hg.mozilla.org/mozilla-central/rev/6cdae42eaa89 https://hg.mozilla.org/mozilla-central/rev/5d9f5afda58e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•