Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Other Branch
mozilla13
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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;");
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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*
Assignee: general → jorendorff
Attachment #552541 - Flags: review?(jimb)

Comment 3

6 years ago
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.
Attachment #552541 - Flags: review?(jimb) → review+

Comment 4

6 years ago
Can we get this landed?
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/42408569a696
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
(Assignee)

Comment 7

6 years ago
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
(Assignee)

Comment 8

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

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ee98e4d71995
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Reporter)

Comment 10

5 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.