328897 - JS_ReportPendingException doesn't

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Add a JS_ReportPendingException call to nsJSThunk::EvaluateScript after the
    call to EvaluateString().
2)  Load the URI in the URL field.

ACTUAL RESULTS:  No error reported

EXPECTED RESULTS: Error reported.

We end up getting into js_ReportUncaughtException, not having a reportp, which makes us call JS_ReportErrorNumber.  Then we get into ReportError, js_ErrorToException succeeds, we notify our debug hook... and nothing.  We never call into cx->errorReporter.

Brendan suggested a hack that makes this "work" locally, but I'm not at all sure it's correct.  I'll attach it in a sec.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Hack

Comment 2

[Brendan Eich [:brendan]]

Attachment: cleaned up hack

I'm trying to get this patch in soon with minimal fuss.  Could have a followup to rename creatingException to noErrorToException, but that would be later.  For now the comment and private nature of the flag are enough to keep me from fretting about any abuses or misunderstandings.  This is all inside-the-engine code.

/be

Comment 3

[Brendan Eich [:brendan]]

Taking.  Wondering about js1.6rc1, whether to fix this little bug for that.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 213704 [details] [diff] [review]
cleaned up hack

r=mrbkap

Comment 5

[Brendan Eich [:brendan]]

Fixed, with this added change:

 JS_ReportPendingException(JSContext *cx)
 {
 #if JS_HAS_EXCEPTIONS
-    JSBool ok;
+    JSBool save, ok;

     CHECK_REQUEST(cx);
-    JS_ASSERT(!cx->creatingException);

     /*
      * Set cx->creatingException to suppress the standard error-to-exception
@@ -4705,9 +4704,10 @@
      * cx->creatingException flag was added to suppress recursive divergence
      * under js_ErrorToException, but it serves for our purposes here too.
      */
+    save = cx->creatingException;
     cx->creatingException = JS_TRUE;
     ok = js_ReportUncaughtException(cx);
-    cx->creatingException = JS_FALSE;
+    cx->creatingException = save;
     return ok;
 #else
     return JS_TRUE;

Thanks to mrbkap for checking me here, bz's conservatism was justified.  The API layering is such that an API client could call JS_ReportPendingExcepion from within itself, or from within some other js_ErrorToException path.  Seems obvious now, not sure why I doubted it.

New patch for branches coming next.

/be

Comment 6

[Brendan Eich [:brendan]]

Bob, is rc1 under way?

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: patch for 1.8* branches

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 213778 [details] [diff] [review]
patch for 1.8* branches

r=mrbkap

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)
> Bob, is rc1 under way?
>
Not yet, but it will be as soon as I finish my verification runs and check on some of the reproducible issues we discussed yesterday. Tongiht I promise I will get cracking on this.

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

I don't see the error in a debug trunk winxp build from today (2006030221) when testing the javascript url above. Are you sure this is fixed? I do see an exception if I try to assign "javascript:Components.classes" in a normal script, but I can't catch it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bob, see step 1 of steps to reproduce.  Current tip doesn't have an error on that URI due to bug 328902.

Comment 13

[Bob Clary [:bc] (inactive)]

ok. not something I can test with the current suite.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 213778 [details] [diff] [review]
patch for 1.8* branches

approved for 1.8.0 branch, a=dveditz for drivers

Comment 15

[Brendan Eich [:brendan]]

Fixed on 1.8.0 branch.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

Checking in regress-328897.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-328897.js,v  <--  regress-328897.js
initial revision: 1.1

the testcase needs a little tweaking to properly detect the exception.

Comment 17

[Bob Clary [:bc] (inactive)]

modify test to handle uncaught exception
http://hg.mozilla.org/mozilla-central/rev/67baf262f121