Fatal JS_Assert "JSVAL_IS_NUMBER(pn3->pn_val) || JSVAL_IS_STRING(pn3->pn_val) || JSVAL_IS_BOOLEAN(pn3->pn_val)"

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: roc, Assigned: Igor Bukanov)

Tracking

({testcase})

Trunk
mozilla1.9beta2
x86
All
testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

281 bytes, text/html
Details
6.55 KB, text/plain
Details
7.77 KB, patch
Igor Bukanov
: review+
Mike Schroepfer
: approvalM10+
Igor Bukanov
: approval1.9+
Details | Diff | Splinter Review
Opening http://www.glazman.org/weblog/dotclear/index.php?post/2007/12/05/Firefox-30-perf-improvements

#0  JS_Assert (s=0x1100790 "JSVAL_IS_NUMBER(pn3->pn_val) || JSVAL_IS_STRING(pn3->pn_val) || JSVAL_IS_BOOLEAN(pn3->pn_val)", file=0x10ffef0 "/Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c", ln=2774) at /Users/roc/mozilla-checkin/mozilla/js/src/jsutil.c:63
#1  0x01045390 in EmitSwitch (cx=0x347bc760, cg=0x4374fe40, pn=0x43625638, stmtInfo=0xbfffc934) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:2772
#2  0x01048e3a in js_EmitTree (cx=0x347bc760, cg=0x4374fe40, pn=0x43625638) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:4185
#3  0x0104bbc8 in js_EmitTree (cx=0x347bc760, cg=0x4374fe40, pn=0x43625610) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:5151
#4  0x01048c11 in js_EmitTree (cx=0x347bc760, cg=0x4374fe40, pn=0x432078c8) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:4141
#5  0x0104bbc8 in js_EmitTree (cx=0x347bc760, cg=0x4374fe40, pn=0x43308ef0) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:5151
#6  0x01046294 in js_EmitFunctionBytecode (cx=0x347bc760, cg=0x4374fe40, body=0x43308ef0) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:3168
#7  0x0104823b in js_EmitTree (cx=0x347bc760, cg=0xbfffd260, pn=0x43308eb8) at /Users/roc/mozilla-checkin/mozilla/js/src/jsemit.c:3969
#8  0x010adfbd in Statements (cx=0x347bc760, ts=0xbfffd3e4, tc=0xbfffd260) at /Users/roc/mozilla-checkin/mozilla/js/src/jsparse.c:1452
#9  0x010ac007 in js_CompileScript (cx=0x347bc760, obj=0x40aaf760, principals=0x439be074, tcflags=4096, chars=0x36573008, length=183662, file=0x0, filename=0x3ecf9e58 "http://www.glazman.org/weblog/dotclear/themes/glazblog/mootools.js", lineno=1) at /Users/roc/mozilla-checkin/mozilla/js/src/jsparse.c:602
#10 0x0102097a in JS_EvaluateUCScriptForPrincipals (cx=0x347bc760, obj=0x40aaf760, principals=0x439be074, chars=0x36573008, length=183662, filename=0x3ecf9e58 "http://www.glazman.org/weblog/dotclear/themes/glazblog/mootools.js", lineno=1, rval=0xbfffd5b8) at /Users/roc/mozilla-checkin/mozilla/js/src/jsapi.c:4854
#11 0x1942a9a4 in nsJSContext::EvaluateString (this=0x4476e570, aScript=@0x45e520b4, aScopeObject=0x40aaf760, aPrincipal=0x439be070, aURL=0x3ecf9e58 "http://www.glazman.org/weblog/dotclear/themes/glazblog/mootools.js", aLineNo=1, aVersion=0, aRetValue=0x0, aIsUndefined=0xbfffd6d4) at /Users/roc/mozilla-checkin/mozilla/dom/src/base/nsJSEnvironment.cpp:1518
#12 0x1929a99a in nsScriptLoader::EvaluateScript (this=0x43ded9c0, aRequest=0x45e520a0, aScript=@0x45e520b4) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsScriptLoader.cpp:616
#13 0x1929ac68 in nsScriptLoader::ProcessRequest (this=0x43ded9c0, aRequest=0x45e520a0) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsScriptLoader.cpp:530
#14 0x1929aced in nsScriptLoader::ProcessPendingRequests (this=0x43ded9c0) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsScriptLoader.cpp:663
#15 0x1929aefd in nsScriptLoader::OnStreamComplete (this=0x43ded9c0, aLoader=0x34744560, aContext=0x45e520a0, aStatus=0, aStringLen=183665, aString=0x355f0008 "/*\nScript: Core.js\n\tMootools - My Object Oriented javascript.\n\nLicense:\n\tMIT-style license.\n\nMooTools Copyright:\n\tcopyright (c) 2007 Valerio Proietti, <http://mad4milk.net>\n\nMooTools Credits:\n\t- Cl"...) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsScriptLoader.cpp:838
#16 0x139a36de in nsStreamLoader::OnStopRequest (this=0x34744560, request=0x34572e90, ctxt=0x45e520a0, aStatus=0) at /Users/roc/mozilla-checkin/mozilla/netwerk/base/src/nsStreamLoader.cpp:108
#17 0x139a30f1 in nsStreamListenerTee::OnStopRequest (this=0x34ee50e0, request=0x34572e90, context=0x45e520a0, status=0) at /Users/roc/mozilla-checkin/mozilla/netwerk/base/src/nsStreamListenerTee.cpp:65
#18 0x13a2ce26 in nsHttpChannel::OnStopRequest (this=0x34572e60, request=0x3f4a1fc0, ctxt=0x0, status=0) at /Users/roc/mozilla-checkin/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:4412
#19 0x1397ea13 in nsInputStreamPump::OnStateStop (this=0x3f4a1fc0) at /Users/roc/mozilla-checkin/mozilla/netwerk/base/src/nsInputStreamPump.cpp:576
#20 0x1397eb31 in nsInputStreamPump::OnInputStreamReady (this=0x3f4a1fc0, stream=0x449b273c) at /Users/roc/mozilla-checkin/mozilla/netwerk/base/src/nsInputStreamPump.cpp:401
#21 0x013c99b4 in nsInputStreamReadyEvent::Run (this=0x34c46f80) at /Users/roc/mozilla-checkin/mozilla/xpcom/io/nsStreamUtils.cpp:111
#22 0x0136eaf7 in nsThread::ProcessNextEvent (this=0x2912760, mayWait=0, result=0xbfffdad4) at /Users/roc/mozilla-checkin/mozilla/xpcom/threads/nsThread.cpp:510
Flags: blocking1.9?
Summary: Fatal JS_Assert JS_Assert (s=0x1100790 "JSVAL_IS_NUMBER(pn3->pn_val) || JSVAL_IS_STRING(pn3->pn_val) || JSVAL_IS_BOOLEAN(pn3->pn_val) → Fatal JS_Assert "JSVAL_IS_NUMBER(pn3->pn_val) || JSVAL_IS_STRING(pn3->pn_val) || JSVAL_IS_BOOLEAN(pn3->pn_val)"

Comment 1

10 years ago
ditto linux
OS: Mac OS X → All

Comment 2

10 years ago
Created attachment 291735 [details]
testcase

Updated

10 years ago
Keywords: testcase

Comment 3

10 years ago
Is that testcase supposed to crash?

Comment 4

10 years ago
(In reply to comment #3)
> Is that testcase supposed to crash?
> 
JS_Assert on a debug build.
This is a nasty one. We should try to get it fixed for beta2 if we possibly can.
I'm seeing this consistently on http://www.mercurynews.com/news/ci_7640472
Target Milestone: --- → mozilla1.9 M10
Created attachment 291797 [details]
my stack
WFM in js shell. Does this crash only in the browser?

Igor, can you take a look? Thanks,

/be
(Assignee)

Updated

10 years ago
Assignee: general → igor

Comment 9

10 years ago
I can't reproduce in the shell either.

Comment 10

10 years ago
+ing and P1 based on Comment 5 - if this is a WFM or otherwise shouldn't block please adjust...
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
(Assignee)

Comment 11

10 years ago
Here is a test case for js shell:

eval("function f(x) { switch (x) { case Array: return 1; }}");
var result = f(Array);
if (result !== 1)
    throw "Unexpected result: "+uneval(result);

Currently it generates:
~/m/trunk/mozilla/js/src $ js ~/m/y.js
Assertion failure: JSVAL_IS_NUMBER(pn3->pn_val) || JSVAL_IS_STRING(pn3->pn_val) || JSVAL_IS_BOOLEAN(pn3->pn_val), at jsemit.c:2774
trace/breakpoint trap

This a bug in the emitter exposed by changes from bug 376957 that made the Array read-only triggering const-propagation code in the emitter when compiling code that will be executed against the given scope.
(Assignee)

Comment 12

10 years ago
On 1.8.1 branch the bug is a harmless assert since the code can deal with arbitrary value for the case and one can fix the bug via removing the assert. On the trunk since resolving the bug 385729 this is not so harmless as the code "officially" no longer supports storing objects as atoms. But in practice it continues to work as the code that access atoms stored in JSScript treats them as the generic jsvals.
Blocks: 385729
(Assignee)

Comment 13

10 years ago
Created attachment 291919 [details] [diff] [review]
fix v1
Attachment #291919 - Flags: review?(brendan)
Comment on attachment 291919 [details] [diff] [review]
fix v1

>                     ok = js_LookupCompileTimeConstant(cx, cg, pn4->pn_atom, &v);
>                     if (!ok)
>                         goto release;
>                     if (!JSVAL_IS_VOID(v)) {
>+                        if (!JSVAL_IS_PRIMITIVE(v)) {
>+                            /*
>+                             * XXX JSOP_LOOKUPSWITCH does not support const-
>+                             * propagated object values, see bug 407186.
>+                             */
>+                            switchOp = JSOP_CONDSWITCH;
>+                            continue;
>+                        }
>                         pn3->pn_val = v;
>                         constPropagated = JS_TRUE;
>                         break;

This break exits the switch to here:

>-            JS_ASSERT(JSVAL_IS_NUMBER(pn3->pn_val) ||
>-                      JSVAL_IS_STRING(pn3->pn_val) ||
>-                      JSVAL_IS_BOOLEAN(pn3->pn_val));
>+            JS_ASSERT(JSVAL_IS_PRIMITIVE(pn3->pn_val));

and the only other cases that break to here are number, string, true, and false. So the change allows null, but not undefined (since js_LookupCompileTimeConstant uses undefined in the v out param to mean "not found"). So the assertion could be tighter, albeit longer.

Fix that if you think it's worth it, and r/a=me.

/be
Attachment #291919 - Flags: review?(brendan)
Attachment #291919 - Flags: review+
Attachment #291919 - Flags: approval1.9+
(Assignee)

Comment 15

10 years ago
Created attachment 291951 [details] [diff] [review]
fix v2

The new version makes LookupCompileTimeConstant a static function that sets *vp to JSVAL_HOLE when it can not find a constant. The patch also makes sure that a read-only, permanent property with a getter is not counted as a source of compile-time constants.
Attachment #291919 - Attachment is obsolete: true
Attachment #291951 - Flags: review?(brendan)
(Assignee)

Comment 16

10 years ago
(In reply to comment #15)
> sets *vp to JSVAL_HOLE when it can not find a constant.

In this way a const holding JSVAL_VOID will be supported. 

Comment on attachment 291951 [details] [diff] [review]
fix v2


>+#define IS_CONSTANT_PROPERTY(attributes)                                      \
>+    (((attributes) & (JSPROP_READONLY | JSPROP_PERMANENT | JSPROP_GETTER)) == \
>+     (JSPROP_READONLY | JSPROP_PERMANENT))

Nit: canonical name is attrs, not attributes.

r+a=me, thanks for the better patch. JSVAL_HOLE ftw!

/be
Attachment #291951 - Flags: review?(brendan)
Attachment #291951 - Flags: review+
Attachment #291951 - Flags: approval1.9+
(Assignee)

Comment 18

10 years ago
Created attachment 291965 [details] [diff] [review]
fix v2b

The new version uses attrs for the argument of the IS_CONSTANT_PROPERTY macro.
Attachment #291951 - Attachment is obsolete: true
Attachment #291965 - Flags: review+
Attachment #291965 - Flags: approval1.9+

Updated

10 years ago
Duplicate of this bug: 407291

Comment 20

10 years ago
k - let's get this now for b2..
Flags: blocking1.9+ → blocking1.9-
Keywords: checkin-needed

Updated

10 years ago
Flags: blocking1.9- → blocking1.9+

Updated

10 years ago
Attachment #291965 - Flags: approvalM10+

Comment 21

10 years ago
Hope no one minds me landing this, since it blocks the B2 code-freeze:

jsemit.c: 3.290
jsemit.h: 3.84
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Thanks, crowder.  Perfect.  :)

Updated

10 years ago
Flags: in-testsuite?
Keywords: checkin-needed

Comment 23

10 years ago
Checking in regress-407024.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-407024.js,v  <--  regress-407024.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+

Comment 24

10 years ago
verified fixed 2007-12-09 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.