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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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);

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jim Blandy :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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Reversed.

Comment 4

[Jim Blandy :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.

Comment 5

[Jason Orendorff [:jorendorff]]

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.

Comment 6

[Jim Blandy :jimb]

Excellent.

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb938ddc272e

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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