Closed Bug 407024 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: roc, Assigned: igor)

References

Details

(Keywords: testcase)

Attachments

(3 files, 2 obsolete files)

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)"
ditto linux
OS: Mac OS X → All
Attached file testcase
Keywords: testcase
Is that testcase supposed to crash?
(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
Attached file my stack
WFM in js shell. Does this crash only in the browser?

Igor, can you take a look? Thanks,

/be
Assignee: general → igor
I can't reproduce in the shell either.
+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
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.
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
Attached patch fix v1 (obsolete) — Splinter Review
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+
Attached patch fix v2 (obsolete) — Splinter Review
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)
(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+
Attached patch fix v2bSplinter Review
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+
Duplicate of this bug: 407291
k - let's get this now for b2..
Flags: blocking1.9+ → blocking1.9-
Keywords: checkin-needed
Flags: blocking1.9- → blocking1.9+
Attachment #291965 - Flags: approvalM10+
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
Closed: 12 years ago
Resolution: --- → FIXED
Thanks, crowder.  Perfect.  :)
Flags: in-testsuite?
Keywords: checkin-needed
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+
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.