Assertion failure: !cx->throwing

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
--
critical
RESOLVED WORKSFORME
7 years ago
5 years ago

People

(Reporter: decoder, Assigned: gal)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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)
(Reporter)

Comment 1

7 years ago
Rechecked this on recent tip, the assertion now is

Assertion failure: !cx->isExceptionPending(), at jscntxtinlines.h:695
Keywords: assertion, testcase
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Updated

7 years ago
Keywords: regression
(Assignee)

Updated

7 years ago
Assignee: general → gal
(Assignee)

Comment 2

7 years ago
Decompiler bug. Not a regression.
Keywords: regression
(Assignee)

Comment 3

7 years ago
We lose the error return value. I think its an easy fix so I will make a patch.
blocking2.0: ? → .x
(Assignee)

Comment 4

7 years ago
Created attachment 506089 [details] [diff] [review]
patch
(Assignee)

Comment 5

7 years ago
Created attachment 506090 [details] [diff] [review]
patch

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
(Assignee)

Updated

7 years ago
Attachment #506090 - Flags: review?(brendan)
(Assignee)

Comment 6

7 years ago
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
(Assignee)

Comment 9

7 years ago
Created attachment 506156 [details] [diff] [review]
patch

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?
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 11

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 676763
(Reporter)

Comment 15

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 16

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