[jsdbg2] Assertion failure: ok, at jsiter.cpp:1574

RESOLVED FIXED in mozilla21

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The following testcase asserts on mozilla-central revision 13fd49ef7786 (run with --ion-eager):


var g = newGlobal('new-compartment');
var dbg = new Debugger(g);
dbg.onDebuggerStatement = function handleDebugger(frame) {
    frame.onPop = function handlePop(c) {
      poppedFrames.indexOf(this)
    }
};
g.eval("function g() { for (var i = 0; i < 10; i++) { debugger; yield i; } }");
assertEq(g.eval("var t = 0; for (j in g()) t += j; t;"), 45);
Posted patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #681758 - Flags: review?(jimb)
When we first discussed the issue on IRC, we decided that having the debugger force a throw from a generator should close the generator, since if the generator had really thrown, it wouldn't be usable any more. That's what this patch implements.

However, in a subsequent discussion we both thought better of it: any object with a 'next' method can be an iterator; and certainly a random object's 'next' might throw, yet have further values to yield. So the argument above isn't protecting any important invariant.

Furthermore, a debugger feature that changes the state of the debuggee should have pretty tight bounds on its effects, to be most useable.

So this patch should be revised to leave the generator usable even when the debugger forces a throw.

Another point: the original test case, and the one included in the patch, turn the generator yield into a throw via an uncaught exception in a Debugger handler function. Does the bug still occur if we take the more direct route of having the handler return a { throw: foo } resumption value? If so, such a test would seem to engage fewer moving parts.
Attachment #681758 - Flags: review?(jimb)
Posted patch v2Splinter Review
Reversed.
Attachment #681758 - Attachment is obsolete: true
Attachment #690919 - Flags: review?(jimb)
Comment on attachment 690919 [details] [diff] [review]
v2

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

It seems like the test is still having the handler throw, instead of returning a { throw: } resumption value as suggested in the last paragraph in comment 2. On IRC, that paragraph's suggestion still seemed like a good idea. So, this is r=me, with that change.
Attachment #690919 - Flags: review?(jimb) → review+
I agree. Both cases are weird enough to warrant a test, though, so I kept the throwing one as Frame-onPop-generators-02, and added the suggested {throw:} version as -01.
Excellent.
https://hg.mozilla.org/mozilla-central/rev/cb938ddc272e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Automatically extracted testcase for this bug was committed:

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