Closed
Bug 1396261
Opened 7 years ago
Closed 7 years ago
ArrayLiteral/ObjectLiteral with property accesses not correctly handled as destructuring assignment targets
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 1 obsolete file)
17.60 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Test case: --- [{a: 0}.x] = []; ({a: {b: 0}.x} = {}); [[0].x] = []; ({a: [0].x} = {}); --- Expected: All four statement are parsed as valid destructuring assignments Actual: All four statements throw a SyntaxError when parsed
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
When we're parsing array elements (or object properties) in a destructuring context, we need to create a new PossibleError for each element/property to handle the case when the element/property is a property accessor with a nested array/object literal. And in Parser::checkDestructuringAssignmentTarget(...), we then need to check for possible expression errors in the property accessor. I've split the arguments/eval name checks from Parser::checkDestructuringAssignmentTarget(...) into the new Parser::checkDestructuringAssignmentName(...) method in order to avoid code duplication when the destructuring name is checked for object shorthand properties (like `{eval} = {}`). That's why the diff got a bit larger than expected. :-/ And it's interesting the fuzzers never hit this problem, because it can easily crash the browser (`[{a = 0}.x] = [];` crashes in http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/js/src/frontend/BytecodeEmitter.cpp#6574).
Attachment #8903938 -
Flags: review?(arai.unmht)
Comment 2•7 years ago
|
||
Comment on attachment 8903938 [details] [diff] [review] bug1396261.patch Review of attachment 8903938 [details] [diff] [review]: ----------------------------------------------------------------- good catch :) The code itself looks good. r+ with the comment fixed. ::: js/src/frontend/Parser.cpp @@ +9486,1 @@ > PossibleError* possibleError, it would be nice to have a comment about the difference between exprPossibleError and possibleError, maybe above this method definition. @@ +9486,5 @@ > PossibleError* possibleError, > TargetBehavior behavior) > { > + // The expression must be either a simple assignment target, i.e. a name > + // or a property accessor, or a nested destructuring pattern. this comment is true only for destructuring context. when this method is called, `expr` can be any expression. @@ +9489,5 @@ > + // The expression must be either a simple assignment target, i.e. a name > + // or a property accessor, or a nested destructuring pattern. > + > + // Report any pending expression error if we're not in a destructuring > + // context or the destructuring target is a property accessor. this comment is a bit misleading. `!possibleError` condition doesn't cover all "we're not in a destructuring context" case. it just means "we're definitely not in a destructuring context". code after this branch is also used by possibly non-destructuring context.
Attachment #8903938 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Updated patch to adjust code comments per comment #2, carrying r+ from :arai.
Attachment #8903938 -
Attachment is obsolete: true
Attachment #8905962 -
Flags: review+
Assignee | ||
Comment 4•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f757ace01ed8acdd25a22968507abe4c74c8525
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d66007719512 Don't treat array/object literals with property accessors as nested destructuring assignment targets. r=arai
Keywords: checkin-needed
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d66007719512
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•