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

RESOLVED FIXED in mozilla21

Status

()

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

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

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

Trunk
mozilla21
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 681758 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #681758 - Flags: review?(jimb)

Comment 2

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

Updated

6 years ago
Attachment #681758 - Flags: review?(jimb)
(Assignee)

Comment 3

6 years ago
Created attachment 690919 [details] [diff] [review]
v2

Reversed.
Attachment #681758 - Attachment is obsolete: true
Attachment #690919 - Flags: review?(jimb)

Comment 4

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

Comment 5

6 years ago
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

6 years ago
Excellent.

Comment 8

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

Comment 9

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