446584 - NodeIterator doesn't forward exception properly

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Michael Ventnor]

Attachment: Testcase

Acid3 test 1 throws the string "Roses" to test whether custom exceptions are passed correctly from the iterator function. We seem to not do this correctly. See the testcase, which is as reduced as I can get it.

Comment 1

[Michael Ventnor]

This might be a problem with XPCExceptions since we're putting too much information in the ToString method, or at least more than what Acid3 expects.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a generic XPConnect problem

Comment 3

[Johnny Stenback (:jst)]

Well, XPConnect wraps all exceptions in XPCOM exception objects and the throws those at the caller instead of only the existing exception, and that way is able to communicate the exception to the caller no matter what language the caller is written in. Given how Acid3 tests this, changing this would require us to stop wrapping exceptions in XPCOM exception objects all together since the test is e === "Roses", so the thrown exception really must be a string with that value, an object that converts to that value wouldn't be enough.

We could change that, I presume, but doing that could likely break existing code, especially chrome code that tests for a particular type of exception today. I wouldn't expect much to break on the web, but who knows with all the browser detecting AJAX libraries and the like out there.

Comment 4

[Johnny Stenback (:jst)]

This is probably worth a try at fixing this problem. I doubt we can pull it off for chrome code, but for web code I think we'd be better off if we don't expose XPConnect stuff to the JS any more than we have to.

Ben Newman agreed to have a look at this. To fix this we'd need to make our XPConnect exception objects hold on to the original jsval for the JS exception (rooted n' all) so that it can be re-thrown later on. The code that sees the original exception is probably nsXPCWrappedJSClass::CheckForException(), and the code that does the conversion from JS exception to XPConnect exception is in XPCConvert::JSValToXPCException(), and the code that should (conditionally) extract the original JS exception and re-throw it should probably live in XPCWrappedNative::CallMethod().

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #4)
> Ben Newman agreed to have a look at this. To fix this we'd need to make our
> XPConnect exception objects hold on to the original jsval for the JS exception
> (rooted n' all) so that it can be re-thrown later on.

Trying to find the best way of storing the jsval.  The method XPCConvert::ConstructException takes a nsISupports* parameter for arbitrary data, which is mostly unused (and the cases where it is used can be disambiguated pretty easily).  If I can pass the jsval as this parameter, I'll be halfway to fixing the bug.

I should be able to wrap the jsval in an object that implements the nsISupports interface, right?  Is that even a reasonable thing to do?

Comment 6

[Johnny Stenback (:jst)]

I don't know of a XPCOM wrapper that can reliably handle wrapping all types of jsvals. Adding a jsval argument to XPCConvert::ConstructException() is probably an easier way to go.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Would an nsIVariant work here?

Comment 8

[Tomas]

Any progress?

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch with more thorough testcase

Notes:

- nsXPCException has two new methods: SetThrownJSVal and GetThrownJSVal.

- I've overloaded XPConvert::ConstructException to take a jsval as its
  final argument (this version of the method just calls the original
  version and then calls SetThrownJSVal on the constructed exception).
  
- Fortunately, nsIExceptions that wrap nsXPCExceptions can be converted
  back into nsXPCExceptions using do_QueryInterface, so I didn't have to
  modify any IDL files.
  
- I wasn't able to #include "nsContentUtils.h" in
  js/src/xpconnect/xpcwrappednative.cpp, so I had to rewrite
  nsContentUtils::IsCallerChrome.
  
- Traceable jsvals are rooted because I store them as XPCTraceableVariants
  inside nsXPCExceptions (non-traceable jsvals are just stored as
  XPCVariants).

My mochitests don't exercise the chrome case (where nothing should have
changed).  Not sure how important that is, but I'm open to suggestions if
there's a good way to check.

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 334385 [details] [diff] [review]
proposed patch with more thorough testcase

Does this fix ACID3/test 1?

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #10)
> (From update of attachment 334385 [details] [diff] [review])
> Does this fix ACID3/test 1?

To my knowledge, yes.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 334385 [details] [diff] [review]
proposed patch with more thorough testcase

>+++ b/js/src/xpconnect/src/xpcprivate.h
>+#define CONSTRUCT_EXCEPTION_FORMALS   \
>+    nsresult rv, const char* message, \
>+    const char* ifaceName,            \
>+    const char* methodName,           \
>+    nsISupports* data,                \
>+    nsIException** exception
>+
>+    static nsresult ConstructException(CONSTRUCT_EXCEPTION_FORMALS);
>+    static nsresult ConstructException(CONSTRUCT_EXCEPTION_FORMALS,
>+                                       jsval jsException);

I think I would prefer if the jsval is not an optional argument. It just makes it easy to forget to pass it in. Especially given how many places you had to pass it in.

Since jsval doesn't really have any value that is "no value", you could make the function take a jsval* and let it be null if no jsval exists. Credit for that idea goes out to mrbkap.

In any case, I'm not sure I like the macro use here. Makes the code hard to read as you have to go find the macro every time you see it.

>+IsCallerChrome() // TODO unify with nsContentUtils::IsCallerChrome?
>+{
>+    PRBool isChrome = PR_FALSE;
>+    nsCOMPtr<nsIScriptSecurityManager> securityManager =
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
>+    nsresult rv = securityManager->SubjectPrincipalIsSystem(&isChrome);
>+    return NS_SUCCEEDED(rv) && isChrome;
>+}

Check with jst if this is the correct way of checking this.

r=me with the ConstructException thing fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Is there a reason we can't use JSVAL_NULL or JSVAL_VOID for the "no value" value?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

The script could technically throw null or undefined.

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #12)
> >+    static nsresult ConstructException(CONSTRUCT_EXCEPTION_FORMALS);
> >+    static nsresult ConstructException(CONSTRUCT_EXCEPTION_FORMALS,
> >+                                       jsval jsException);
> 
> I think I would prefer if the jsval is not an optional argument. It just makes
> it easy to forget to pass it in. Especially given how many places you had to
> pass it in.

So the burden will be on callers to opt out of storing a jsval in the exception.  I'm on board with that.  Especially since I think I see a way to avoid the do_QueryInterface song&dance if I consolidate the two versions of ConstructException.

> In any case, I'm not sure I like the macro use here. Makes the code hard to
> read as you have to go find the macro every time you see it.

Fair enough.  The only motivation for the macro was to emphasize the overloading, which makes sense only in the .h file anyway.  It was definitely a mistake to use the macro in the .cpp file.  But of course there's no sense in using the macro at all if we go back to having just one version of ConstructException.  Point taken.

> >+IsCallerChrome() // TODO unify with nsContentUtils::IsCallerChrome?
> >+{
> >+    PRBool isChrome = PR_FALSE;
> >+    nsCOMPtr<nsIScriptSecurityManager> securityManager =
> >+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
> >+    nsresult rv = securityManager->SubjectPrincipalIsSystem(&isChrome);
> >+    return NS_SUCCEEDED(rv) && isChrome;
> >+}
> 
> Check with jst if this is the correct way of checking this.

Will do.

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

Some patches landed since I posted my patch seem to have invalidated my code for extracting the exception in XPCWrappedNative::CallMethod.  Still looking into that.  If anyone knows better, let me know.

Comment 17

[Ben Newman (:bnewman) (:benjamn)]

Attachment: review comments addressed, fastpath changes accounted for

Comment 18

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 337743 [details] [diff] [review]
review comments addressed, fastpath changes accounted for

Moved the extraction logic into XPCThrower::ThrowExceptionObject, which is used both by the quickstubs and by XPCWrappedNative::CallMethod.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 337743 [details] [diff] [review]
review comments addressed, fastpath changes accounted for

- In XPCConvert::ConstructException():

+    if (NS_SUCCEEDED(res) && jsExceptionPtr && *exceptn) {
+        nsCOMPtr<nsXPCException> xpcEx = do_QueryInterface(*exceptn);
+        if (xpcEx)
+            xpcEx->SetThrownJSVal(*jsExceptionPtr);

Please follow surrounding style, ugly as it is :). Drop the space-after-if, and put the opening curly brace on its own line (non-indented).

- In nsXPCException::GetThrownJSVal():

+    if (mThrownJSVal) {

Above style comment applies here.

+        if (vp) *vp = mThrownJSVal->GetJSVal();

break that on two lines please. And actually, there's no need to if check vp here, all callers always pass in a non-null pointer.

+        return PR_TRUE;
+    } else return PR_FALSE;

else-after-return, loose the else.

sr=jst with that, and Jonas says his r= still stands.

Comment 20

[Ben Newman (:bnewman) (:benjamn)]

Attachment: stylistic fixups

@jst @jonas thanks for the reviews!

Comment 21

[Ben Newman (:bnewman) (:benjamn)]

Attachment: removing stray space-after-if

lamest patch revision ever :)

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 337996 [details] [diff] [review]
removing stray space-after-if

+nsXPCException::GetThrownJSVal(jsval *vp) const
+{
+    if(mThrownJSVal)
+    {
+        if(vp)

I'd still argue that this check is not needed! :) Is there a real reason for needing this, or is this a just-in-case check? If the latter, I'd argue for removing the check. Either way is ok, just wondering...

Comment 23

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #22)
> (From update of attachment 337996 [details] [diff] [review])
> +nsXPCException::GetThrownJSVal(jsval *vp) const
> +{
> +    if(mThrownJSVal)
> +    {
> +        if(vp)
> 
> I'd still argue that this check is not needed! :) Is there a real reason for
> needing this, or is this a just-in-case check? If the latter, I'd argue for
> removing the check. Either way is ok, just wondering...

I think originally I imagined it might be useful to pass null to GetThrownJSVal just to test whether a jsval had been set, without having to declare a dummy jsval.  None of my other code exercises this use-case, though, I admit.

Comment 24

[Bill Gianopoulos [:WG9s]]

(In reply to comment #21)
> Created an attachment (id=337996) [details]
> removing stray space-after-if

First, the good news.  My Linux build including this patch passes Acid3 Test 1.
Now the bad news.  My Windows build including this patch fails Acid3 Test 1.

Comment 25

[Bill Gianopoulos [:WG9s]]

(In reply to comment #24)
> (In reply to comment #21)
> > Created an attachment (id=337996) [details] [details]
> > removing stray space-after-if
> 
> First, the good news.  My Linux build including this patch passes Acid3 Test 1.
> Now the bad news.  My Windows build including this patch fails Acid3 Test 1.

Nevermind.  I had screwed up in applying the patch under Windows.

Sorry for the SPAM.

Comment 26

[Bill Gianopoulos [:WG9s]]

As it turns out, this does not compile under windows.  It seems to need the following additional fix:

--- a/js/src/xpconnect/src/XPCDispTearOff.cpp   Fri Sep 12 16:59:39 2008 -0700
+++ b/js/src/xpconnect/src/XPCDispTearOff.cpp   Fri Sep 12 21:03:40 2008 -0400
@@ -460,17 +460,17 @@
             char* sz = nsnull;

             if(nsXPCException::NameAndFormatForNSResult(code, nsnull, &msg) &&
msg)
                 sz = JS_smprintf(format, msg, name);

             nsCOMPtr<nsIException> e;

             XPCConvert::ConstructException(code, sz, "IDispatch", name.get(),
-                                           nsnull, getter_AddRefs(e));
+                                           nsnull, getter_AddRefs(e), nsnull);
             xpcc->SetException(e);
             if(sz)
                 JS_smprintf_free(sz);
         }

         if(!success)
         {
             retval = nsXPCWrappedJSClass::CheckForException(ccx, name.get(),

Comment 27

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #26)
> As it turns out, this does not compile under windows.  It seems to need the
> following additional fix:
> 
>              XPCConvert::ConstructException(code, sz, "IDispatch", name.get(),
> -                                           nsnull, getter_AddRefs(e));
> +                                           nsnull, getter_AddRefs(e), nsnull);

How right you are!  This oversight would prevent it compiling on any platform, I would think.  I should have done a clean build before I submitted the patch, but looking at mxr.mozilla.org now I don't see any other ConstructException call sites where I forgot to update the argument list.  Removed the checkin-needed keyword until I can submit an emended patch.

I guess you weren't building from scratch on Linux?

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to comment #27)
> (In reply to comment #26)
> > As it turns out, this does not compile under windows.  It seems to need the
> > following additional fix:
> > 
> >              XPCConvert::ConstructException(code, sz, "IDispatch", name.get(),
> > -                                           nsnull, getter_AddRefs(e));
> > +                                           nsnull, getter_AddRefs(e), nsnull);
> 
> How right you are!  This oversight would prevent it compiling on any platform,
> I would think.  I should have done a clean build before I submitted the patch,
> but looking at mxr.mozilla.org now I don't see any other ConstructException
> call sites where I forgot to update the argument list.  Removed the
> checkin-needed keyword until I can submit an emended patch.
> 
> I guess you weren't building from scratch on Linux?

Evidently, the Linux build is only giving a warning on this.  It was c clean build.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

XPCDispTearOff is only built on Windows.  (Well, you technically could change that with --disable-xpconnect-idispatch or --enable-xpconnect-idispatch, but I don't think it would compile on non-Windows.)

Comment 30

[Ben Newman (:bnewman) (:benjamn)]

Attachment: corrected xpcdisptearoff.cpp [Checkin: Comment 31]

making Bill's change

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 338678 [details] [diff] [review]
corrected xpcdisptearoff.cpp
[Checkin: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/3a86a986c037

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So... The XPCVariants used in SetThrownJSVal cause us to hit the cycle collector from non-main threads. We gotta fix this somehow since exceptions are supposed to be threadsafe (and workers use them a bunch!).

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #32)

Filed bug 462389 to keep the noise down in here.