The patch in bug 1059754 (attachment 8480613 [details] [diff] [review]) triggers this assertion, that might be related to the array comprehension there: Assertion failure: pn->pn_u.list.head->isKind(PNK_LEXICALSCOPE), at /Users/paolo/m-c/js/src/jsreflect.cpp:3001 To reproduce, execute these steps in a debug build: 1. Apply the patch in bug 1065446 2. Apply the patch in bug 1059754 3. Run the command: ./mach mochitest-browser --setpref parse=MozLoop browser/base/content/test/general/browser_parsable_script.js

This line is the current suspect: callback(...[for (r of results) cloneValueInto(r, targetWindow)]);

Just a : Cu.import("resource://gre/modules/reflect.jsm"); Reflect.parse("callback(...[for (r of results) cloneValueInto(r, targetWindow)])"); verifies that this is indeed what's causing the issue here. I don't know why this works in practice but breaks in reflect.jsm...

Reflect.parse is a complete reimplementation of the parse-tree-walking performed when SpiderMonkey produces bytecode for a parse tree. If any of the walking doesn't handle a parse node's possible child kinds correctly -- and it's fairly easy for this to happen in very complex bytecode, which certainly is the case whenever generator expressions and block scopes are involved -- we generally assert and crash. The run-the-code walking is entirely different. If it handles all the kid kinds correctly, actually using such code wouldn't crash. This all is probably what's happening here.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > Reflect.parse is a complete reimplementation of the parse-tree-walking > performed when SpiderMonkey produces bytecode for a parse tree. If any of > the walking doesn't handle a parse node's possible child kinds correctly -- > and it's fairly easy for this to happen in very complex bytecode, which > certainly is the case whenever generator expressions and block scopes are > involved -- we generally assert and crash. The run-the-code walking is > entirely different. If it handles all the kid kinds correctly, actually > using such code wouldn't crash. This all is probably what's happening here. Is there an alternative way of getting the parse tree that matches 1:1 with what spidermonkey actually does? The point of the "check if our CSS parses" test is that, um, we want to know that all of it is OK, parsing-wise (and ideally, we'll add more logic on top of the AST at some stage). If the parse tree that reflect.parse generates and the tree that spidermonkey *actually* generates are not the same, that's going to be sadfaces lots of the time.

s/CSS/JS/, obviously. D'oh.

I changed the line to the non-standard comprehension syntax and the assertion did not show up anymore. callback(...[cloneValueInto(r, targetWindow) for (r of results)]);

(In reply to :Gijs Kruitbosch from comment #4) > (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > > Reflect.parse is a complete reimplementation of the parse-tree-walking > > performed when SpiderMonkey produces bytecode for a parse tree. > > Is there an alternative way of getting the parse tree that matches 1:1 with > what spidermonkey actually does? The point of the "check if our CSS parses" > test is that, um, we want to know that all of it is OK, parsing-wise (and > ideally, we'll add more logic on top of the AST at some stage). If the parse > tree that reflect.parse generates and the tree that spidermonkey *actually* > generates are not the same, that's going to be sadfaces lots of the time. You misread. There is only one generated parse tree. There are two parse tree *walking* implementations: one used for generating bytecode, one for Reflect.parse. While in theory there could also be only one, through a very, very massive visitor-pattern template or so, there is not now. Given the eccentricity of our parse tree right now, I kind of doubt it could work for generating bytecode in any event. It might be possible after further refactoring of the parse tree to be less crazy. So, no, you don't have any other options right now. But this is pretty easily fixed, so I don't think you need to worry about this as a long-term or frequent concern.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7) > But this is pretty easily fixed, so I don't think you need to worry about this as a long-term > or frequent concern. Out of curiosity, what would be required to fix this bug? I worked around it in comment 6, so fixing this is not urgent, but it seems to me that this bug would prevent our chrome JS from using the standard syntax for comprehensions, so we'll want to fix it eventually.

(In reply to :Paolo Amadini from comment #8) > Out of curiosity, what would be required to fix this bug? Not much more than adding a little "fork" into control flow, at the point of this assertion, to handle the alternative parse tree/syntax. Off the top of my head, I don't know exact contours of the parse tree for new-style generators, so I can't describe the fix much more precisely than that. I don't have the time to fix this myself right now, but I could probably mentor a fix, for someone who's willing to look at Parser.cpp and do some learning about how the parse tree for this is set up. Ideally it wouldn't even take that -- this structure *should* be described in the comments in ParseNode.h -- but I'm having trouble understanding the PNK_ARRAYCOMP comments that should explain this. (Those could probably use a little cleanup here, too.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > I don't have the time to fix this myself right now, but I could probably > mentor a fix, for someone who's willing to look at Parser.cpp and do some > learning about how the parse tree for this is set up. Thanks! Sounds like a [good next bug] for someone interested in the JavaScript Engine? Feel free to correct the tags if I'm underestimating the complexity here.

Tradeoffs: the easiest bugs for new contributors are assertion failures, but we can't just leave an assertion failure to languish in the codebase. No choice but to fix them ourselves as time allows.

Comment on attachment 8497521 [details] [diff] [review] Make Reflect.parse properly handle new-style array comprehensions and generator expressions Review of attachment 8497521 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsreflect.cpp @@ +669,5 @@ > bool comprehensionBlock(HandleValue patt, HandleValue src, bool isForEach, bool isForOf, TokenPos *pos, > MutableHandleValue dst); > > bool comprehensionExpression(HandleValue body, NodeVector &blocks, HandleValue filter, > + bool isOldStyle, TokenPos *pos, MutableHandleValue dst); isLegacy or isLegacyStyle or something, everywhere. We're pretty consistent about using "legacy" to denote these. @@ +1417,5 @@ > if (!newArray(blocks, &array)) > return false; > > + RootedValue style(cx); > + if (!atomValue(isOldStyle ? "old" : "new", &style)) legacy/modern might be the better, along previous lines. @@ +1441,5 @@ > if (!newArray(blocks, &array)) > return false; > > + RootedValue style(cx); > + if (!atomValue(isOldStyle ? "old" : "new", &style)) legacy/modern @@ +2612,1 @@ > while (next->isKind(PNK_FOR)) { If we're LOCAL_ASSERTing at the start, please flip this into a do-while loop, so the code doesn't admit a case the assertion claims will never happen. @@ +2638,5 @@ > > bool > ASTSerializer::generatorExpression(ParseNode *pn, MutableHandleValue dst) > { > + bool isOldStyle = pn->isKind(PNK_LEXICALSCOPE); Maybe a "// Just as there are two kinds of array comprehension (see ASTSerializer::comprehension), there are two kinds of generator expression." to explain this. @@ +2648,1 @@ > while (next->isKind(PNK_FOR)) { Same do-while thing. ::: js/src/tests/js1_8_5/extensions/reflect-parse.js @@ +855,3 @@ > > +// Comprehension expressions using for-of can be written in two different styles. > +function assertCompExpr(expr, body, blocks, filter) { Maybe fold "legacy" into the name somehow. @@ +859,5 @@ > > + // Transform the old-style comprehension to a new-style comprehension and > + // test it that way too. > + let expr2 = expr.replace(/^\[(.*?) for (.*)\]$/, function (m, g1, g2) { > + return '[for ' + g2 + ' ' + g1 + ']'; Moderate fan of var matches = expr.match(/^\[(.*?) for (.*)\]$/); assertEq(matches !== null, true); var expr2 = "[for " + matches[2] + " " + matches[1] + "]"; or somesuch. A little strange, feels like to me, to replace the entire string (with function replacement, even!), versus extracting out the interesting parts and reassembling into the modern ordering. @@ +931,5 @@ > > + // Transform the old-style genexpr to a new-style genexpr and test it that > + // way too. > + let expr2 = expr.replace(/^\((.*?) for (.*)\)$/, function (m, g1, g2) { > + return '(for ' + g2 + ' ' + g1 + ')'; Same sort of adjustments as for the assertCompExpr function.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14) > @@ +2612,1 @@ > > while (next->isKind(PNK_FOR)) { > > If we're LOCAL_ASSERTing at the start, please flip this into a do-while > loop, so the code doesn't admit a case the assertion claims will never > happen. I didn't take this suggestion, because there's a downside too, to do with reasoning about code. When you see 'while (COND) STMT', you know immediately that COND is always true on entry to STMT. Rewriting as 'assert(COND); blahblah; do STMT while (COND)' requires the reader to check the duplicate COND and make sure it's really the same in both places. Applied all the rest. Try servering in a few minutes. Thanks.