Closed
Bug 1107592
Opened 9 years ago
Closed 9 years ago
Error.name is clobbered when throwing DOMError to content from JSImplemented webidl
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jib, Assigned: bzbarsky)
References
()
Details
Attachments
(2 files)
4.44 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
12.41 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8532242 -
Flags: review?(peterv)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
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+
Reporter | ||
Comment 6•9 years ago
|
||
It inherits name from Error, does it not?
Assignee | ||
Comment 7•9 years ago
|
||
> 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 8•9 years ago
|
||
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+
Reporter | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
> 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
Assignee | ||
Comment 11•9 years ago
|
||
And Ehsan (thanks!) pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f22e7b280329 to fix non-unified build bustage.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fea8c83aef60 https://hg.mozilla.org/mozilla-central/rev/0738d2b29e8b https://hg.mozilla.org/mozilla-central/rev/f22e7b280329
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
•