Closed Bug 1107592 Opened 5 years ago Closed 5 years ago

Error.name is clobbered when throwing DOMError to content from JSImplemented webidl

Categories

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

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jib, Assigned: bzbarsky)

References

()

Details

Attachments

(2 files)

STR: Click URL.

Expected result: NotSupportedError: updateIce not yet implemented line 28

Actual result: Error: updateIce not yet implemented line 28

Expected result is needed to implement RTCPeerConnection error-handling to the WebRTC spec (affects other methods, updateIce is just the simplest example).

Workaround: While I've been able to work around it poorly for promise-returning methods (Bug 1106675 comment 0) there appears to be no good workaround in the non-promise (throw) case.
So in this case, per spec, you want to throw an actual DOMException, right?

We should be able to make this work.  Patch coming up.
Assignee: nobody → bzbarsky
The idea here is that if you're supposed to throw a DOMException you just create one with the right name and message via the constructor on the content window, and then bindings will adjust the filename and line number to make it look like it was thrown at the API entrypoint.
Attachment #8532243 - Flags: review?(peterv)
(In reply to Boris Zbarsky [:bz] from comment #1)
> So in this case, per spec, you want to throw an actual DOMException, right?

Yes, my assessment is that that's where we're headed, since promises drive us there. http://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors

The spec http://w3c.github.io/webrtc-pc currently specifies DOMError but my understanding is that the webrtc group is just waiting for the DOM team to come out and settle the DOMError/DOMException debate.
Blocks: 1107953
No longer blocks: 1106675
See Also: → 1111679
Comment on attachment 8532242 [details] [diff] [review]
part 1.  Implement a DOMException constructor

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

::: dom/base/DOMException.cpp
@@ +685,5 @@
> +  nsresult exceptionResult = NS_OK;
> +  uint16_t exceptionCode = 0;
> +  nsCString name(NS_LITERAL_CSTRING("Error"));
> +
> +  if (aName.WasPassed()) {

So the name argument is not actually in the spec, is there a bug on that? If it's not planned to be added to the spec we should check for chrome-only here I think.
Attachment #8532242 - Flags: review?(peterv) → review+
It inherits name from Error, does it not?
> So the name argument is not actually in the spec, is there a bug on that?

https://github.com/heycam/webidl/pull/22

> It inherits name from Error, does it not?

No.  "name" on Error is an own property on the prototype with the value ""; if you create an Error instance you have to manually define "name" on it to get any other value.
Comment on attachment 8532243 [details] [diff] [review]
part 2.  Allow chrome JS to directly throw content DOMExceptions that will propagate out to the web script

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

::: dom/bindings/BindingUtils.cpp
@@ +248,5 @@
> +                            domException->Code());
> +    JS::Rooted<JS::Value> reflector(aCx);
> +    if (!GetOrCreateDOMReflector(aCx, newException, &reflector)) {
> +      // Well, that threw _an_ exception.  Let's forget ours.
> +      js::RemoveRawValueRoot(aCx, &mJSException);

Hmm, it's a bit unclear what mJSException is going to be now. Do we want it to be undefined or the thrown exception? Please document.
Attachment #8532243 - Flags: review?(peterv) → review+
Fyi, odd bitrot in part 1 I think, affecting Linux debug only, from https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57dc7863097 :
> 13:07:51 INFO - In file included from /builds/slave/try-lx-d-000000000000000000000/build/src/dom/base/DOMException.cpp:6:0:
> 13:07:51 INFO - ../../dist/include/mozilla/dom/DOMException.h:140:15: error: 'GlobalObject' has not been declared
> 13:07:51 INFO - ../../dist/include/mozilla/dom/DOMException.h:142:21: error: 'Optional' does not name a type
> 13:07:51 INFO - ../../dist/include/mozilla/dom/DOMException.h:142:21: error: ISO C++ forbids declaration of 'parameter' with no type [-fpermissive]
> 13:07:51 INFO - ../../dist/include/mozilla/dom/DOMException.h:142:29: error: expected ',' or '...' before '<' token
> Fyi, odd bitrot in part 1 I think

Just a missing forward-declare; I added it locally after pushing to try.

> Hmm, it's a bit unclear what mJSException is going to be now

It doesn't matter, since we're changing mResult to NS_ERROR_FAILURE so we will not examine mJSException again.  Documented.  That said, this code is missing a return right after we set mResult, because we want to avoid calling ReportJSException if "reflector" doesn't exist!  Added that too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fea8c83aef60
https://hg.mozilla.org/integration/mozilla-inbound/rev/0738d2b29e8b
Flags: in-testsuite+
Target Milestone: --- → mozilla37
And Ehsan (thanks!) pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f22e7b280329 to fix non-unified build bustage.
Depends on: 1131894
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.