678086 - [jsdbg2] Assertion failure: fp->isScriptFrame(), at vm/Debugger.cpp:393

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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]]

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]]

Attachment: 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]

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]

Can we get this landed?

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42408569a696

Comment 6

[Joe Drew (not getting mail)]

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]]

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]]

Trying again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee98e4d71995

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ee98e4d71995

Comment 10

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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