Last Comment Bug 328897 - JS_ReportPendingException doesn't
: JS_ReportPendingException doesn't
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
javascript:window.location="javascrip...
Depends on:
Blocks: js1.6rc1 328851
  Show dependency treegraph
 
Reported: 2006-02-28 15:01 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2009-02-20 17:42 PST (History)
5 users (show)
dveditz: blocking1.8.0.4+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Hack (859 bytes, patch)
2006-02-28 15:01 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
cleaned up hack (1.14 KB, patch)
2006-03-01 18:52 PST, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Splinter Review
patch for 1.8* branches (1.17 KB, patch)
2006-03-02 12:09 PST, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2006-02-28 15:01:12 PST
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 [:bz] (still a bit busy) 2006-02-28 15:01:39 PST
Created attachment 213504 [details] [diff] [review]
Hack
Comment 2 Brendan Eich [:brendan] 2006-03-01 18:52:36 PST
Created attachment 213704 [details] [diff] [review]
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] 2006-03-01 22:58:14 PST
Taking.  Wondering about js1.6rc1, whether to fix this little bug for that.

/be
Comment 4 Blake Kaplan (:mrbkap) 2006-03-02 11:29:11 PST
Comment on attachment 213704 [details] [diff] [review]
cleaned up hack

r=mrbkap
Comment 5 Brendan Eich [:brendan] 2006-03-02 12:08:02 PST
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] 2006-03-02 12:08:36 PST
Bob, is rc1 under way?

/be
Comment 7 Brendan Eich [:brendan] 2006-03-02 12:09:26 PST
Created attachment 213778 [details] [diff] [review]
patch for 1.8* branches
Comment 8 Blake Kaplan (:mrbkap) 2006-03-02 12:09:53 PST
Comment on attachment 213778 [details] [diff] [review]
patch for 1.8* branches

r=mrbkap
Comment 9 Bob Clary [:bc:] 2006-03-02 12:40:57 PST
(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] 2006-03-02 12:51:39 PST
Fixed on the 1.8 branch.

/be
Comment 11 Bob Clary [:bc:] 2006-03-02 22:33:36 PST
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 [:bz] (still a bit busy) 2006-03-02 22:47:20 PST
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:] 2006-03-02 22:50:37 PST
ok. not something I can test with the current suite.
Comment 14 Daniel Veditz [:dveditz] 2006-04-03 12:14:40 PDT
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] 2006-04-22 23:12:59 PDT
Fixed on 1.8.0 branch.

/be
Comment 16 Bob Clary [:bc:] 2006-06-26 12:01:46 PDT
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:] 2009-02-20 17:42:52 PST
modify test to handle uncaught exception
http://hg.mozilla.org/mozilla-central/rev/67baf262f121

Note You need to log in before you can comment on or make changes to this bug.