Closed Bug 328897 Opened 19 years ago Closed 19 years ago

JS_ReportPendingException doesn't


(Core :: JavaScript Engine, defect, P2)






(Reporter: bzbarsky, Assigned: brendan)




(Keywords: fixed1.8.0.4, fixed1.8.1)


(3 files)


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.
Attached patch HackSplinter Review
OS: Linux → All
Hardware: PC → All
Version: 1.8 Branch → Trunk
Attached patch cleaned up hackSplinter Review
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.

Attachment #213704 - Flags: review?(mrbkap)
Taking.  Wondering about js1.6rc1, whether to fix this little bug for that.

Assignee: general → brendan
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 213704 [details] [diff] [review]
cleaned up hack

Attachment #213704 - Flags: review?(mrbkap) → review+
Fixed, with this added change:

 JS_ReportPendingException(JSContext *cx)
-    JSBool ok;
+    JSBool save, ok;

-    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;
     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.

Closed: 19 years ago
Flags: blocking1.8.0.3?
Resolution: --- → FIXED
Bob, is rc1 under way?

Blocks: js1.6rc1
Attachment #213778 - Flags: review?(mrbkap)
Attachment #213778 - Flags: approval1.8.0.3?
Attachment #213778 - Flags: approval-branch-1.8.1+
Comment on attachment 213778 [details] [diff] [review]
patch for 1.8* branches

Attachment #213778 - Flags: review?(mrbkap) → review+
(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.
Fixed on the 1.8 branch.

Keywords: fixed1.8.1
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.
Bob, see step 1 of steps to reproduce.  Current tip doesn't have an error on that URI due to bug 328902.
ok. not something I can test with the current suite.
Flags: in-testsuite-
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 213778 [details] [diff] [review]
patch for 1.8* branches

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #213778 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fixed on 1.8.0 branch.

Keywords: fixed1.8.0.3
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.
Flags: in-testsuite- → in-testsuite+
You need to log in before you can comment on or make changes to this bug.