Last Comment Bug 769754 - Crash while using the debugger
: Crash while using the debugger
Status: RESOLVED FIXED
[js:p1]
: crash
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on: 772770
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-29 12:07 PDT by Paul Rouget [:paul]
Modified: 2012-07-16 11:55 PDT (History)
8 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue. (2.16 KB, patch)
2012-07-04 11:13 PDT, Jim Blandy :jimb
jorendorff: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Comment 1 Panos Astithas [:past] 2012-06-29 12:59:31 PDT
Paul, any specific STR?
Comment 2 Paul Rouget [:paul] 2012-07-01 10:58:52 PDT
More crashes:
- https://crash-stats.mozilla.com/report/index/bp-125ab2b3-88b3-4cd9-87f3-536bb2120701
- https://crash-stats.mozilla.com/report/index/b368d8e5-c164-48df-8b9e-6755b2120701
- https://crash-stats.mozilla.com/report/index/bp-7a18e055-9dbe-4a4b-8568-ad50a2120701

This happens almost every time I use the debugger.

STR: Stepping through this code crashes Firefox:

  function run() {
    let e = "f o o o b b b b b b b b  a a a r".split(" ");
    for (let c of e) {
      console.log(e);
    }
  }
Comment 3 Panos Astithas [:past] 2012-07-02 02:04:52 PDT
I've added the code from comment 2 in a test page:

http://htmlpad.org/step-crash/

After setting a breakpoint at line 7 and stepping over 4 times, I get an assertion:

Assertion failure: !cx->iterValue.isMagic(JS_NO_ITER_VALUE), at /Users/past/src/fx-team/js/src/jsiter.cpp:1280

The full trace is here, but it doesn't look quite like Paul's:

http://past.pastebin.mozilla.org/1687533

Paul, if you can reproduce your exact crash/trace with this test page, please add the STR here and I'll file a different bug.
Comment 4 Paul Rouget [:paul] 2012-07-02 02:32:52 PDT
Break point line 6. Step in 4 time. Crash:
https://crash-stats.mozilla.com/report/index/bp-d86fd7b3-4338-4f9f-8e74-66a032120702
Comment 5 Paul Rouget [:paul] 2012-07-02 02:39:01 PDT
With Past's STR (line 7, step over 4 times), I get this crash:
https://crash-stats.mozilla.com/report/index/bp-f53c6b06-1222-42bc-9e16-438d22120702
Comment 6 Panos Astithas [:past] 2012-07-02 02:41:44 PDT
(In reply to Paul Rouget [:paul] from comment #4)
> Break point line 6. Step in 4 time. Crash:
> https://crash-stats.mozilla.com/report/index/bp-d86fd7b3-4338-4f9f-8e74-
> 66a032120702

Following these steps in my fx-team tip debug build results in the same crash as in comment 3, so the different stacks could probably be explained by paul's optimized build versus my own debug one.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-07-03 07:00:02 PDT
does this occur in Aurora as well?
Comment 8 Jason Orendorff [:jorendorff] 2012-07-03 08:11:39 PDT
Here's what I tried to reproduce this in the shell, but it runs correctly.

var g = newGlobal('new-compartment');
g.eval("var line0 = Error().lineNumber;\n" +
       "function run() {\n" +
       "    let e = 'f o o o b b b b b b b b  a a a r'.split(' ');\n" +
       "    for (let c of e) {\n" +
       "        print(e);\n" +
       "    }\n" +
       "}\n");

var dbg = new Debugger;
var gw = dbg.addDebuggee(g);
var runw = gw.getOwnPropertyDescriptor('run').value;
var bp = {
    hit: function (frame) {
        print("hit");
        frame.onStep = function () {
            print("stepped, line " + this.script.getOffsetLine(this.offset));
        };
    }
};
for (let off of runw.script.getLineOffsets(g.line0 + 2))
    runw.script.setBreakpoint(off, bp);

g.run();
Comment 9 Jim Blandy :jimb 2012-07-03 10:57:08 PDT
I can work on this.
Comment 10 Jim Blandy :jimb 2012-07-03 16:42:47 PDT
All Paul's crashes are in the JSON.stringify method, as called from an onStep handler; the debug server is probably preparing a packet to send to the client. That's probably why Jason's code doesn't reproduce the problem.

Panos's crash appears to be entirely unrelated.
Comment 11 Jim Blandy :jimb 2012-07-03 23:01:32 PDT
I can reproduce Panos's crash. It is suspicious that the bad value Panos's crash finds in cx->iterValue (0x02) is the same as the bad pointer being followed in Paul's crash, so let's not split out the crashes into separate bugs just yet.
Comment 12 Panos Astithas [:past] 2012-07-04 08:11:06 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> does this occur in Aurora as well?

Yep.
Comment 13 Jim Blandy :jimb 2012-07-04 09:22:52 PDT
Panos' crash, at least, is caused by Debugger::onStepHandler not preserving the value of cx->iterValue, a temporary slot used when invoking iterators. The debuggee sets it to a string, but then Debugger::onStepHandler runs; does something with iterators that wipes out cx->iterValue with its own value, which is eventually MagicValue(JS_NO_ITER_VALUE); and then returns to the debuggee, which (correctly) asserts that it has a legitimate value in there.

This should be easy to reproduce in the shell.
Comment 14 Jim Blandy :jimb 2012-07-04 09:39:19 PDT
This reproduces Panos's crash.

var g = newGlobal('new-compartment');
var dbg = new Debugger;
var gw = dbg.addDebuggee(g);
var log;
var a = [];

dbg.onDebuggerStatement = function (frame) {
  print('d');
  log += 'd';
  frame.onStep = function () {
    print('s');
    // This handler must not wipe out the debuggee's value in JSContext::iterValue.
    log += 's';
    // This will use JSContext::iterValue in the debugger.
    for (let i of a)
      log += 'i';
  };
};

log = '';
g.eval("debugger; for (let i of [1,2,3]) print(i);");
assertEq(log.match(/ds*/));
Comment 15 Jim Blandy :jimb 2012-07-04 11:13:31 PDT
Created attachment 639146 [details] [diff] [review]
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue.

A patch; testing now.
Comment 16 Jim Blandy :jimb 2012-07-04 11:16:19 PDT
Paul, were your crashes from a browser built with DEBUG?
Comment 17 Paul Rouget [:paul] 2012-07-04 11:28:20 PDT
(In reply to Jim Blandy :jimb from comment #16)
> Paul, were your crashes from a browser built with DEBUG?

No.
Comment 18 Jim Blandy :jimb 2012-07-04 11:40:32 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=f80cdcb6cc4c
Comment 19 Jim Blandy :jimb 2012-07-04 15:56:59 PDT
Fixes the problem in the browser.
Comment 20 Jim Blandy :jimb 2012-07-04 15:59:07 PDT
"The problem" may be too optimistic. Paul, when you get a chance, could you see if the attached patch fixes the crash for you? I can't get a crash any more. You said on IRC that you were using a non-debug build, so hopefully the difference in backtraces is due to Panos's and my debug builds catching the problem earlier, and the attached patch will address both manifestations.
Comment 21 Jim Blandy :jimb 2012-07-04 16:03:45 PDT
Comment on attachment 639146 [details] [diff] [review]
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue.

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

Try run looks good.
Comment 22 Jason Orendorff [:jorendorff] 2012-07-06 21:47:40 PDT
Comment on attachment 639146 [details] [diff] [review]
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue.

This is the grossest thing ever. We've got to get that opcode sequence changed. Barf.
Comment 23 Jim Blandy :jimb 2012-07-09 13:31:26 PDT
Pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5dacb3e58bd
Comment 24 Jim Blandy :jimb 2012-07-09 13:42:10 PDT
Comment on attachment 639146 [details] [diff] [review]
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
single-stepping support in Debugger

User impact if declined:
Users: none. Web developers: stepping through a 'for-in' loop in script debugger crashes Firefox. (Embarrassing. We have many single-step unit tests, but none in which the handler itself also executes a 'for-in' loop. This patch adds such a test.)

Testing completed (on m-c, etc.):
Try server run linked to in bug. Landed on inbound; will let it bake a bit.

Risk to taking this patch (and alternatives if risky):
Most of the affected code paths are only executed by the script debugger. The sole exception is a newly added assertion, which is debug-only.

String or UUID changes made by this patch: none
Comment 25 Paul Rouget [:paul] 2012-07-10 10:49:41 PDT
Can't reproduce with inbound. \o/
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:50:06 PDT
https://hg.mozilla.org/mozilla-central/rev/a5dacb3e58bd
Comment 27 Alex Keybl [:akeybl] 2012-07-12 15:04:32 PDT
Comment on attachment 639146 [details] [diff] [review]
Debugger handler functions should not corrupt the debuggee's JSContext::iterValue.

[Triage Comment]
Given user risk vs dev reward, approving for Aurora 15.
Comment 28 Panos Astithas [:past] 2012-07-16 04:30:29 PDT
I landed it in Aurora to make sure we don't miss the uplift:
https://hg.mozilla.org/releases/mozilla-aurora/rev/018027a303aa
Comment 29 Jim Blandy :jimb 2012-07-16 11:53:21 PDT
(In reply to Panos Astithas [:past] from comment #28)
> I landed it in Aurora to make sure we don't miss the uplift:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/018027a303aa

Thanks, Panos!

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