ArrayLiteral/ObjectLiteral with property accesses not correctly handled as destructuring assignment targets

RESOLVED FIXED in Firefox 57



JavaScript Engine
9 months ago
8 months ago


(Reporter: anba, Assigned: anba)



Firefox Tracking Flags

(firefox57 fixed)



(1 attachment, 1 obsolete attachment)



9 months ago
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


9 months ago
Assignee: nobody → andrebargull

Comment 1

9 months ago
Created attachment 8903938 [details] [diff] [review]

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
Attachment #8903938 - Flags: review?(arai.unmht)
Comment on attachment 8903938 [details] [diff] [review]

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+

Comment 3

9 months ago
Created attachment 8905962 [details] [diff] [review]

Updated patch to adjust code comments per comment #2, carrying r+ from :arai.
Attachment #8903938 - Attachment is obsolete: true
Attachment #8905962 - Flags: review+

Comment 5

8 months ago
Pushed by
Don't treat array/object literals with property accessors as nested destructuring assignment targets. r=arai
Keywords: checkin-needed
Last Resolved: 8 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.