Closed Bug 621988 Opened 9 years ago Closed 8 years ago

Assertion failure: !cx->throwing

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED WORKSFORME
Tracking Status
blocking2.0 --- .x+

People

(Reporter: decoder, Assigned: gal)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(1 file, 2 obsolete files)

Shell testcase

f = function() { L: 3; alert(5); }
print(f.toString(28800000,23,59,999));

triggers Assertion failure: !cx->throwing, at jscntxtinlines.h:687

(Tested on mc trunk)
Rechecked this on recent tip, the assertion now is

Assertion failure: !cx->isExceptionPending(), at jscntxtinlines.h:695
Keywords: assertion, testcase
blocking2.0: --- → ?
Keywords: regression
Assignee: general → gal
Decompiler bug. Not a regression.
Keywords: regression
We lose the error return value. I think its an easy fix so I will make a patch.
blocking2.0: ? → .x
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
There is a couple different ways of doing this. We could also try to recover the error code by checking cx->isExceptionPending(). That makes a shorter patch. This is our standard way though. Brendan, what do you think?
Attachment #506089 - Attachment is obsolete: true
Attachment #506090 - Flags: review?(brendan)
Great testcase by the way. Thanks.
Comment on attachment 506090 [details] [diff] [review]
patch

>+#define JS_PRINTF(jp, s, a0)                                                  \
>+    JS_BEGIN_MACRO                                                            \
>+    if (js_printf(jp, s, a0) < 0)                                             \
>+            return false;                                                     \

Indentation is off on the return line, here and elsewhere.

This is too intrusive a patch. Rather, poison jp (it has three bools in a row, add another) and AND that error flag with the decompiler's nominal result.

/be
Attachment #506090 - Flags: review?(brendan) → review-
(In reply to comment #0)
> Shell testcase
> 
> f = function() { L: 3; alert(5); }
> print(f.toString(28800000,23,59,999));

Comment 7 last paragraph may be worth doing, but there's an even more direct fix: clamp the indent argument to Function.prototype.toString using a sane limit.

ECMA-262 says not to extend built-in methods with arguments not in the spec, but this extension is ancient, possibly primordial. We should not remove it at this point in the release, and we'd need a deprecation release to do so (it can remain a parameter of toSource).

Clamping won't hurt anyone though.

/be
Attached patch patchSplinter Review
This fixes the underlying bug. We can in addition add a limit, I certainly don't mind.
Attachment #506090 - Attachment is obsolete: true
Attachment #506156 - Flags: review?
Attachment #506156 - Flags: review? → review?(brendan)
Comment on attachment 506156 [details] [diff] [review]
patch

>@@ -2324,18 +2328,17 @@ Decompile(SprintStack *ss, jsbytecode *p
>                     jp2 = js_NewPrinter(cx, "nested_function", fun,
>                                         jp->indent, jp->pretty, jp->grouped,
>                                         jp->strict);
>                     if (!jp2)
>                         return NULL;
>                     ok = js_DecompileFunction(jp2);
>                     if (ok && jp2->sprinter.base)
>                         js_puts(jp, jp2->sprinter.base);
>-                    js_DestroyPrinter(jp2);
>-                    if (!ok)
>+                    if (!js_DestroyPrinter(jp2) || !ok)
>                         return NULL;

Slightly prettier to do

>.                    ok &= js_DestroyPrinter(jp2);
>.                    if (!ok)
>.                        return NULL;

r=me with that.

/be
Attachment #506156 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/7da52f991c29
Whiteboard: fixed-in-tracemonkey
Backed out, js_DestroyPrinter had a bool argument that made jsd cry when it somehow #included jsopcode.h, and changing to JSBool seemingly turned everything orange.  But that might also have been me from a followup-fix push (no compiled code touched!) I made at the same time.

Anyway: fix the C-versus-C++ thing, and look at:

http://tbpl.mozilla.org/?tree=TraceMonkey&rev=b2024811aea7

and see if that was your fault, my bad attempt to fix it, or something else before retrying.  (I'll try to get to that before you do as at least partial penance, but I can't guarantee I'll be back to hacking before you will.)
Whiteboard: fixed-in-tracemonkey
My relanding of everything I had was green, so I think the mega-orange was due to this patch.
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/7da52f991c29
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.
Blocks: 676763
I cannot reproduce this anymore on tip. Bisect shows

The first good revision is:
changeset:   70661:707cefa34478
user:        Alon Zakai <azakai@mozilla.com>
date:        Fri Jun 03 17:54:26 2011 -0700
summary:     Bug 644241, part 1 - Remove script stack quota. r=igor


Closing as WFM.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.