Last Comment Bug 678086 - [jsdbg2] Assertion failure: fp->isScriptFrame(), at vm/Debugger.cpp:393
: [jsdbg2] Assertion failure: fp->isScriptFrame(), at vm/Debugger.cpp:393
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: mozilla13
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2011-08-10 16:02 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:14 PST (History)
6 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.84 KB, patch)
2011-08-11 16:43 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2011-08-10 16:02:10 PDT
The following code crashes on jsdbg2 branch (revision 48e43edc8834, options -j -m):


function f() {
    frame = dbg.getNewestFrame();
}
var g = newGlobal('new-compartment');
g.f = function (a, b) { return a + "/" + this . f( ) ; };
var dbg = Debugger(g);
dbg.onDebuggerStatement = function (frame) {
    var f = frame.eval("f").return;
    assertEq(f.call(null, "a", "b").return, "a/b");
};
g.eval("debugger;");
Comment 1 Jason Orendorff [:jorendorff] 2011-08-11 16:37:47 PDT
Reduced:

var g = newGlobal('new-compartment');
g.f = function () { dbg.getNewestFrame(); };
var dbg = new Debugger;
dbg.addDebuggee(g).getOwnPropertyDescriptor("f").value.call();

Boring patch coming.
Comment 2 Jason Orendorff [:jorendorff] 2011-08-11 16:43:22 PDT
Created attachment 552541 [details] [diff] [review]
v1

The bug is that Debugger::observesFrame doesn't know we never want to observe dummy frames. It has three call sites, only one of which correctly calls !fp->isDummyFrame() first. In particular, the asserting test case has us calling Debugger::getYoungestFrame, which does not check for dummy frames.

So: Add an isDummyFrame() call in observesFrame and remove the now-redundant one in Debugger::getOlder.

*yawn*
Comment 3 Jim Blandy :jimb 2011-09-22 14:28:31 PDT
Comment on attachment 552541 [details] [diff] [review]
v1

Review of attachment 552541 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to have a comment in Debugger::observesFrame.
Comment 4 Jim Blandy :jimb 2012-01-12 21:43:16 PST
Can we get this landed?
Comment 5 Jason Orendorff [:jorendorff] 2012-01-27 12:17:59 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/42408569a696
Comment 6 Joe Drew (not getting mail) 2012-01-28 20:02:06 PST
Philor backed bug 695922, bug 678086, and bug 719841 out of mozilla-inbound for causing Tp crashes.
https://hg.mozilla.org/mozilla-central/rev/117f2280bd37
Comment 7 Jason Orendorff [:jorendorff] 2012-02-01 14:08:34 PST
Of course I was unable to reproduce the crash by running talos locally. But looking closer at the patch made it clear. The test case to reproduce this crash turns out to be the following smiley face:

    ({c:

and the fix is like this:

diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -7013,26 +7013,26 @@ Parser::primaryExpr(TokenKind tt, bool a
                 reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_BAD_PROP_ID);
                 return NULL;
             }
 
             op = JSOP_INITPROP;
             tt = tokenStream.getToken();
             if (tt == TOK_COLON) {
                 pnval = assignExpr();
+                if (!pnval)
+                    return NULL;
 
                 /*
                  * Treat initializers which mutate __proto__ as non-constant,
                  * so that we can later assume singleton objects delegate to
                  * the default Object.prototype.
                  */
-                if ((pnval && !pnval->isConstant()) ||
-                    atom == context->runtime->atomState.protoAtom) {
+                if (!pnval->isConstant() || atom == context->runtime->atomState.protoAtom)
                     pn->pn_xflags |= PNX_NONCONST;
-                }
             }
 #if JS_HAS_DESTRUCTURING_SHORTHAND
             else if (ltok == TOK_NAME && (tt == TOK_COMMA || tt == TOK_RC)) {
                 /*
                  * Support, e.g., |var {x, y} = o| as destructuring shorthand
                  * for |var {x: x, y: y} = o|, per proposed JS2/ES4 for JS1.8.
                  */
                 tokenStream.ungetToken();
diff --git a/js/src/jit-test/tests/basic/bug678086-syntax.js b/js/src/jit-test/tests/basic/bug678086-syntax.js
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug678086-syntax.js
@@ -0,0 +1,2 @@
+load(libdir + "asserts.js");
+assertThrowsInstanceOf(function () { eval("({p:"); }, SyntaxError);  // don't crash
Comment 8 Jason Orendorff [:jorendorff] 2012-02-02 05:30:44 PST
Trying again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee98e4d71995
Comment 9 Ed Morley [:emorley] 2012-02-03 11:42:21 PST
https://hg.mozilla.org/mozilla-central/rev/ee98e4d71995
Comment 10 Christian Holler (:decoder) 2013-02-07 05:14:25 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.