Closed
Bug 621988
Opened 13 years ago
Closed 13 years ago
Assertion failure: !cx->throwing
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
blocking2.0 | --- | .x+ |
People
(Reporter: decoder, Assigned: gal)
Details
(Keywords: assertion, testcase)
Attachments
(1 file, 2 obsolete files)
5.29 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Rechecked this on recent tip, the assertion now is Assertion failure: !cx->isExceptionPending(), at jscntxtinlines.h:695
Assignee | ||
Updated•13 years ago
|
blocking2.0: --- → ?
Assignee | ||
Updated•13 years ago
|
Keywords: regression
Assignee | ||
Updated•13 years ago
|
Assignee: general → gal
Assignee | ||
Comment 3•13 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•13 years ago
|
||
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
Attachment #506090 -
Flags: review?(brendan)
Assignee | ||
Comment 6•13 years ago
|
||
Great testcase by the way. Thanks.
Comment 7•13 years ago
|
||
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-
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
Attachment #506156 -
Flags: review? → review?(brendan)
Comment 10•13 years ago
|
||
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•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/7da52f991c29
Whiteboard: fixed-in-tracemonkey
Comment 12•13 years ago
|
||
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
Comment 13•13 years ago
|
||
My relanding of everything I had was green, so I think the mega-orange was due to this patch.
Comment 14•13 years ago
|
||
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 | ||
Comment 15•13 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
Closed: 13 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 16•11 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.
Description
•