Closed Bug 905392 Opened 7 years ago Closed 7 years ago

Need way to throw web-console-visible exception-message from JS-implemented webidl object

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 - fixed
firefox26 - ---

People

(Reporter: jib, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

In order to fix Bug 903741, I think I need a fix or feature in webidl, as I'm unable to find a way that works today.

I need to throw a custom error message inside a constructor or method call in a JS-implemented webidl browser-object and (if uncaught in my code) have that message make it back to content JS (try/catch), as well as be seen in web console if uncaught by caller.

I've tried, without success:

  throw new Components.Exception(msg);
  throw new Error(msg);
  throw new win.DOMError(errmsg, msg);
  throw publify(new Components.Exception(msg)); // a prop-copy w/Cu.createObjectIn 

I see that https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Throwing_exceptions_from_WebIDL_methods.2C_getters.2C_and_setters covers how to throw an exception from C++ using the ErrorHandler interface, but it doesn't mention how to throw out of the API when using JSImplementation.

Since Bug 903741 is a regression, a fix that was easy to uplift to Aurora would be ideal.
Ugh.  We never actually got anywhere with our exception story for js-implemented stuff.  :(

Basically, we need a way to tell apart "this is an exception to report to the calling web code" and "I threw an unexpected exception".  We just haven't figured out such a way yet...
> Basically, we need a way to tell apart "this is an exception to report to the
> calling web code" and "I threw an unexpected exception".  We just haven't figured
> out such a way yet...

Is there a security issue with returning information in the latter case? (from your comment I assume yes).

I can see a number of ways (throwing out ideas):

 1. Add an ErrorResult arg like in c++ (yuck), or

 2. Only neuter/wrap exception-objects that aren't already webidl-objects, or

 3. [Throws="Error, DOMError"]

With number 2 I would expect this to get out to content:

  throw new this._win.DOMError(errorMsg + " - improper scheme: " + url.scheme);

In fact it seemed so promising to me I had to try if it already worked, but no luck. I get this in the new Browser console (which is awesome btw - Command+Shift+j):

  uncaught exception: [object XrayWrapper [object DOMError]]
Also tried:

  throw new this._win.wrappedJSObject.DOMError(errorMsg);
  throw new this._win.wrappedJSObject.Error(errorMsg);

Got these respectively (still in Browser console):

  uncaught exception: [object DOMError]
  Error: RTCPeerConnection passed invalid RTCConfiguration - improper scheme: html

That might be a 4th idea: Let through unwrapped objects.
> Is there a security issue with returning information in the latter case? 

In general, I think so, yes.  More importantly, we have no way to do it right now: if we tried to just rethrow the chrome exception, I _think_ we'd end up with an opaque object content-side and it wouldn't show any information at all in the error console.  Bobby, is that correct?

So what might be simplest is to check whether the exception is an Xray for a content exception and if so rethrow it to content instead of reporting it in the chrome and throwing an opaque exception in the content.  The JS_WrapValue into the content compartment would produce the right thing in this case, I think, and then the chrome code could do the "throw new win.DOMError(errmsg, msg);" thing.  Again, I think.
Flags: needinfo?(bobbyholley+bmo)
(In reply to Boris Zbarsky [:bz] from comment #4)
> In general, I think so, yes.  More importantly, we have no way to do it
> right now: if we tried to just rethrow the chrome exception, I _think_ we'd
> end up with an opaque object content-side and it wouldn't show any
> information at all in the error console.  Bobby, is that correct?

Yes. But in general for all WebIDL stuff, we should avoid direct JS->JS edges to any chrome-implemented JS.

> So what might be simplest is to check whether the exception is an Xray for a
> content exception

You mean like TypeError? We don't have Xrays for such things.

> and if so rethrow it to content instead of reporting it in
> the chrome and throwing an opaque exception in the content.  The
> JS_WrapValue into the content compartment would produce the right thing in
> this case, I think, and then the chrome code could do the "throw new
> win.DOMError(errmsg, msg);" thing.  Again, I think.

I think that the WebIDL bindings should handle this by inspecting any exceptions that cross their boundaries, and either producing C++ objects for them that can be reflected into the appropriate scope, or creating a JS exception object directly in the calling scope. IMO we should not be using wrappers here.
Flags: needinfo?(bobbyholley+bmo)
> You mean like TypeError?

No, I mean like a DOMError.

> I think that the WebIDL bindings should handle this by inspecting any exceptions that
> cross their boundaries

My point is that if the chrome code just has a bug that causes it to throw, we do NOT want to report all the file/line/message info about that to the content code: that leaks information about the chrome code.

Which means we need a way for chrome code to opt in to "I'm now throwing an exception that should in fact be propagated to the content".  The question is what that way should be.

You're right that just directly rethrowing the content exception is no good, but we could do something where if we see an Xray for a DOMError we get its message and name and use that to throw a useful exception back to the content code.
Ah, I see.

Does JS-implemented WebIDL always have a reference to the content window? If so, this should all work, given how we jiggered things in bug 861493.
JS-implemented WebIDL always has access to the window the "content-side" object is associated with.  That might be a chrome window, of course.   So maybe we should actually check for just a maybe-wrapped DOMError...
I think we should check to see if the exception, when wrapped into the content-side scope, turns out not to be a wrapper at all - that is to say, it's canonical scope is in content-land.
Bug 905392 isn't landed and isn't tracking so I don't see a reason to track this bug while it's being worked out - please do nominate a low-risk solution for uplift if/when available but I don't see the user-impact that would lead us to hold the FF25 release for this.
User impact is in bug 903741. The user in this case being web makers learning WebRTC in Firefox.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
This seems to mostly work. Having to use the .wrappedJSObject to get to the Error constructor is annoying though. Xrays really should let through standard classes IMHO, but maybe we should use DOMError for now?
(In reply to Peter Van der Beken [:peterv] from comment #12)
> Created attachment 797496 [details] [diff] [review]
> v1
> 
> This seems to mostly work. Having to use the .wrappedJSObject to get to the
> Error constructor is annoying though.

Yeah, I don't think we should be doing that.

> Xrays really should let through
> standard classes IMHO

That's kind of a can of worms, TBH, because most standard class constructors create pure JS objects without any kind of recognizable C++ representation that we can Xray to.

> but maybe we should use DOMError for now?

That seems ideal. Is that incompatible with the spec?
> but maybe we should use DOMError for now?

The problem is that afaict the web console doesn't actually show DOMError in any sort of nice way (e.g. showing the error message!).  Maybe we need to get that fixed, but doing that for Aurora 25 sounds chancy.
> Having to use the .wrappedJSObject to get to the Error constructor is annoying
> though.

But yay it works! - Love it for Aurora to avoid regressing. :-)

  Error: RTCPeerConnection constructor passed invalid RTCConfiguration -
  improper scheme: html @ .../components/PeerConnection.js:383

One nit, could we have file+line point to the re-throw point (api) rather than the original file/line? I think that would be desirable, since it is an extension of the api.

  e.g.  @ .../test.html:161

>> but maybe we should use DOMError for now?
>
>That seems ideal. Is that incompatible with the spec?

The webrtc spec? Should be fine. Webrtc is moving toward DOMError http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcerror

> The problem is that afaict the web console doesn't actually show DOMError in any
> sort of nice way (e.g. showing the error message!).

Verified:
  uncaught exception: [object DOMError]

> Maybe we need to get that fixed, but doing that for Aurora 25 sounds chancy.

Is doing it one way for Aurora and another for trunk an option?
> One nit, could we have file+line point to the re-throw point (api)

The only way to do that would be to extract the string from the original Error object and throw a new exception.  The file+line is captured when the Error object is created, I guess.

I do think we want to try to do something like that, though: exposing the chrome-side file+line is not good.  :(

> Is doing it one way for Aurora and another for trunk an option?

Maybe, but for now we should focus on a patch we can get backported, land it on trunk, and then worry about doing something else, I think.
> The only way to do that would be to extract the string from the original Error
> object and throw a new exception.

Sounds fine for backport I think.
Not just for backport: it's the only viable solution in general...
I think I'm going to try this: look for a content DOMError thrown from the JS implementation, extract the message and throw a JS error from that. That'll avoid wrappedJSObject and still give us good information in the console. Now I just need to figure out how to do that.
> Webrtc is moving toward DOMError

Draft was updated today to reflect embrace of DOMError http://dev.w3.org/2011/webrtc/editor/webrtc.html#callback-rtcpeerconnectionerrorcallback-parameters
(In reply to Peter Van der Beken [:peterv] from comment #19)
> I think I'm going to try this: look for a content DOMError thrown from the
> JS implementation, extract the message and throw a JS error from that.
> That'll avoid wrappedJSObject and still give us good information in the
> console. Now I just need to figure out how to do that.

That is an excellent idea. I'm pretty wary of using wrappedJSObject in DOM API implementations. What if new Error() returned some sinister proxy? What kind of mischief might it cause?
Attached patch v2 (obsolete) — Splinter Review
Attachment #797496 - Attachment is obsolete: true
Comment on attachment 798058 [details] [diff] [review]
v2

I'm not able to compile this version of the patch. More changes coming?

.../dom/bindings/CallbackObject.cpp:158:23: error: no matching function for call to 'UnwrapObject'
  return NS_SUCCEEDED(UnwrapObject<DOMError>(mCx, obj, domError));
                      ^~~~~~~~~~~~~~~~~~~~~~
...
../../dist/include/mozilla/dom/BindingUtils.h:200:1: note: candidate template ignored: invalid explicitly-specified argument for template parameter
      'PrototypeID'
UnwrapObject(JSContext* cx, JSObject* obj, U& value)
^
That's because of bug 908576.

What you want is to replace "UnwrapObject<DOMError>(mCx, obj, domError)" with "UNWRAP_OBJECT(DOMError, mCx, obj, domError)".
Attached patch v2.1Splinter Review
Attachment #798058 - Attachment is obsolete: true
Attachment #798849 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #24)
> That's because of bug 908576.
> 
> What you want is to replace "UnwrapObject<DOMError>(mCx, obj, domError)"
> with "UNWRAP_OBJECT(DOMError, mCx, obj, domError)".

Oddly enough, it compiles for me on Linux and on OS X. I'll make that change though.
> Oddly enough, it compiles for me on Linux and on OS X.

Against current tip?
Comment on attachment 798849 [details] [diff] [review]
v2.1

>+++ b/dom/bindings/BindingUtils.cpp
>+#include "mozilla/dom/DOMErrorBinding.h"

You shouldn't need this include anymore, now that we killed the prototype id map stuff.

>+ErrorResult::ReportJSExceptionFromJSImplementation(JSContext* cx)
>+  dom::UnwrapObject<dom::DOMError>(cx, &mJSException.toObject(), domError);

Can you please NS_RUNTIMEABORT if !domError here, with a comment pointing to the fact that when this code is reached if there is a JS exception at all it must have unwrapped to DOMError?  I'd really like us to not even potentially do that virtual function call on null.

Also, this will need to become the UNWRAP_OBJECT macro, which will need fixing to work outside the dom namespace.

>+  mResult = NS_ERROR_FAILURE;

And document that this is needed because we no longer have a useful mJSException?

>+++ b/dom/bindings/CallbackObject.cpp
>+#include "mozilla/dom/DOMErrorBinding.h"

Again, this #include is not needed.

>+CallbackObject::CallSetup::RethrowException(JS::Handle<JS::Value> aException)

I think we should call this ShouldRethrowException.

> CallbackObject::CallSetup::~CallSetup()
>+  if (js::GetObjectCompartment(obj) != mCompartment) {

It's worth a comment above this line that says we want to only rethrow if the exception is a DOMError in the caller compartment.

>+++ b/dom/bindings/CallbackObject.h
>+    eRethrowContentExceptions,

This could use documentation.

>+              JSCompartment* aCompartment = nullptr);

And this.

>+    JSCompartment* mCompartment;

And this.

>+++ b/dom/bindings/Codegen.py
>+class CGJSImplMember(CGNativeMember):

Might be good to add a docstring here.

>+class CGJSImplMethod(CGJSImplMember):

And I know this didn't use to have one, but would be nice too.

>+    def getArgs(self, returnType, argList):
>+        args = CGNativeMember.getArgs(self, returnType, argList)
>+        if not self.isConstructor:

How about:

  if self.isConstructor:
    # Skip the JSCompartment bits for constructors; it's handled
    # manually in getConstructorImpl.
    return CGNativeMember.getArgs(self, returnType, argList)
  return CGJSImplMember.getArgs(self, returnType, argList)

or something?  That said, I'm a bit confused here: the code in getImpl() passes a JSCompartment* as the first argument, no?  Where is that picked 

>-            # The first two arguments to the constructor implementation are not
>+            # The second and third arguments to the constructor implementation are not

This comment change seems wrong.  Leftovers from a previous patch version?

>     def getCallSetup(self):
>+            callSetup += ", eRethrowContentExceptions, aCompartment"

Maybe add a comment that in this case there is no aExceptionHandling argument at all, so it's clear why we're not using it here?

>+++ b/dom/bindings/ErrorResult.h
>@@ -72,16 +72,17 @@ public:
>+  void ReportJSExceptionFromJSImplementation(JSContext* cx);

Needs documentation.

r=me with those nits fixed.  Thank you for doing this!
Attachment #798849 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #28)
> That said, I'm a bit confused here: the code in getImpl()
> passes a JSCompartment* as the first argument, no?  Where is that picked 

I'm not sure what you mean. getImpl uses getArgs (which adds the JSCompartment* for non-constructor members) and it computes the JSCompartment* itself for constructor members.
Flags: needinfo?(bzbarsky)
Er, I meant to delete that whole bit from "That said" to "picked", because I figured it out as I was writing it; that's why it's not a complete sentence.  Clearly the attempt to delete it failed.  :(
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/6c7c1a5007a4
https://hg.mozilla.org/mozilla-central/rev/9fa7a3f9e4d6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
I've asked for Aurora approval in Bug 903741 which depends on this bug. If someone who knows the risks better than I do, could do the same here that would be great! Thanks.
Flags: needinfo?(peterv)
Attached patch Aurora patchSplinter Review
Note: this is a prerequisite for fixing bug 903741. If that one is approved this one should be too.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 823512
User impact if declined: see bug 903741
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): pretty low-risk, this adds infrastructure to throw exceptions to content with relevant information, as opposed to the cryptic exceptions that we have now
String or IDL/UUID changes made by this patch: none
Attachment #802383 - Flags: approval-mozilla-aurora?
Flags: needinfo?(peterv)
Comment on attachment 802383 [details] [diff] [review]
Aurora patch

Approving in support of bug 903741
Attachment #802383 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.