Closed Bug 446584 Opened 12 years ago Closed 12 years ago

NodeIterator doesn't forward exception properly

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: ventnor.bugzilla, Assigned: mozilla+ben)

References

Details

(Keywords: testcase)

Attachments

(2 files, 4 obsolete files)

Attached file 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.
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.
This is a generic XPConnect problem
Component: DOM: Traversal-Range → XPConnect
QA Contact: traversal-range → xpconnect
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.
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().
Assignee: nobody → bnewman
(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?
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.
Would an nsIVariant work here?
Any progress?
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.
Attachment #334385 - Flags: superreview?
Attachment #334385 - Flags: review?(jonas)
Attachment #334385 - Flags: superreview? → superreview?(jst)
Comment on attachment 334385 [details] [diff] [review]
proposed patch with more thorough testcase

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

To my knowledge, yes.
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.
Is there a reason we can't use JSVAL_NULL or JSVAL_VOID for the "no value" value?
The script could technically throw null or undefined.
(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.
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.
Attachment #334385 - Attachment is obsolete: true
Attachment #337743 - Flags: review?(jonas)
Attachment #334385 - Flags: superreview?(jst)
Attachment #337743 - Flags: superreview?(jst)
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 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.
Attachment #337743 - Flags: superreview?(jst)
Attachment #337743 - Flags: superreview+
Attachment #337743 - Flags: review?(jonas)
Attachment #337743 - Flags: review+
Attached patch stylistic fixups (obsolete) — Splinter Review
@jst @jonas thanks for the reviews!
Attachment #337743 - Attachment is obsolete: true
Attachment #337994 - Flags: superreview?(jst)
Attachment #337994 - Flags: review?(jonas)
Attachment #337994 - Flags: superreview?(jst)
Attachment #337994 - Flags: review?(jonas)
Attached patch removing stray space-after-if (obsolete) — Splinter Review
lamest patch revision ever :)
Attachment #337994 - Attachment is obsolete: true
Attachment #337996 - Flags: review+
Keywords: checkin-needed
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...
(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.
(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.
(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.
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(),
(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?
Keywords: checkin-needed
(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.
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.)
making Bill's change
Attachment #337996 - Attachment is obsolete: true
Attachment #338678 - Flags: review+
Keywords: checkin-needed
Comment on attachment 338678 [details] [diff] [review]
corrected xpcdisptearoff.cpp
[Checkin: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/3a86a986c037
Attachment #338678 - Attachment description: corrected xpcdisptearoff.cpp → corrected xpcdisptearoff.cpp [Checkin: Comment 31]
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
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!).
(In reply to comment #32)

Filed bug 462389 to keep the noise down in here.
Depends on: 467865
You need to log in before you can comment on or make changes to this bug.