JS_ReportPendingException doesn't

RESOLVED FIXED in mozilla1.9alpha1



JavaScript Engine
12 years ago
9 years ago


(Reporter: bz, Assigned: brendan)


({fixed1.8.0.4, fixed1.8.1})

fixed1.8.0.4, fixed1.8.1
Dependency tree / graph
Bug Flags:
blocking1.8.0.4 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)




(3 attachments)


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.
Created attachment 213504 [details] [diff] [review]


12 years ago
OS: Linux → All
Hardware: PC → All
Version: 1.8 Branch → Trunk

Comment 2

12 years ago
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.

Attachment #213704 - Flags: review?(mrbkap)

Comment 3

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

Assignee: general → brendan
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha


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

Attachment #213704 - Flags: review?(mrbkap) → review+

Comment 5

12 years ago
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.

Last Resolved: 12 years ago
Flags: blocking1.8.0.3?
Resolution: --- → FIXED

Comment 6

12 years ago
Bob, is rc1 under way?

Blocks: 309169

Comment 7

12 years ago
Created attachment 213778 [details] [diff] [review]
patch for 1.8* branches
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+

Comment 9

12 years ago
(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

12 years ago
Fixed on the 1.8 branch.

Keywords: fixed1.8.1

Comment 11

12 years ago
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.

Comment 13

12 years ago
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+

Comment 15

12 years ago
Fixed on 1.8.0 branch.

Keywords: fixed1.8.0.3

Comment 16

11 years ago
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+

Comment 17

9 years ago
modify test to handle uncaught exception
You need to log in before you can comment on or make changes to this bug.