1651732 - Accessing a proxy's property using a Symbol returned from a implicit object[Symbol.toPrimitive] throws "TypeError: can't convert symbol to string"

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[AK GRK]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.116 Safari/537.36

Steps to reproduce:

const object = {
Symbol.toPrimitive {
return(Symbol());
}
};

const proxy = (new Proxy({},{
get(object,property,proxy) {
return(property);
}
}));

//fails when c>=10
for (let c = 0;(c < 15);c++) {
try {
console.log(c,proxy[object]);
} catch (error) {
console.log(c,error);
}
}

works on: Firefox 50.0-52.9.0, Chrome
fails on: Firefox 53.0-78.0.1, Nightly 80.0a1

Windows 10 Pro 1909 64bits

Actual results:

Console output:
0 Symbol()
1 Symbol()
...
9 Symbol()
10 TypeError: can't convert symbol to string
11 TypeError: can't convert symbol to string
...
14 TypeError: can't convert symbol to string

Expected results:

0 Symbol()
1 Symbol()
...
14 Symbol()

Comment 1

[AK GRK]

Attachment: Untitled.png

Comment 2

[AK GRK]

Attachment: Untitled2.png

Comment 3

[AK GRK]

Attachment: Untitled3.png

Comment 4

[André Bargull [:anba]]

Doesn't reproduce with --no-blinterp, so probably some issue in CacheIR.

Comment 5

[André Bargull [:anba]]

I guess this happens:

• We get into megamorphic state and call GetPropIRGenerator::tryAttachGenericProxy.
• CacheIRCompiler::emitCallProxyGetByValueResult calls into js::ProxyGetPropertyByValue.
• js::ProxyGetPropertyByValue in turn uses ValueToId, which basically calls ToString instead of ToPrimitive.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 1: Change ProxyGetPropertyByValue to use ToPropertyKey. r=jandem!

ValueToId() calls ToAtom() for objects, which isn't correct per spec,
instead ToPropertyKey() needs to be used here.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 2: Change ProxySetPropertyByValue to use ToPropertyKey. r=jandem!

Similar to part 1, replace ToValueId() with ToPropertyKey().

Depends on D83052

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 3: Change ProxyHas to use ToPropertyKey. r=jandem!

Similar to part 1, replace ToValueId() with ToPropertyKey().

Depends on D83053

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 4: Change ProxyHasOwn to use ToPropertyKey. r=jandem!

Similar to part 1, replace ToValueId() with ToPropertyKey().

Depends on D83054

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 5: Change IonGetPropSuperIC::update to use ToPropertyKey. r=jandem!

Similar to part 1, replace ToValueId() with ToPropertyKey().

Depends on D83055

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 6: Change DebuggerObject to use ToPropertyKey. r=jandem!

Similar to part 1, replace ToValueId() with ToPropertyKey().

Depends on D83056

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 7: Change additional callers to use ToPropertyKey instead of ValueToId. r=jandem!

These changes should result in no observable behaviour for their current usage.

intrinsic_DefineDataProperty:

• Using ToPropertyKey also is a better match for the inlined, three argument
  form of _DefineDataProperty.

JS_ValueToId:

• It looks like this function is only called with String values, so changing it
  to use ToPropertyKey won't result in any observable difference.

js::ValueToIdentifier:

• This function will throw an error when symbols are encountered anyway, so we
  might as well change it to use ToPropertyKey.

Depends on D83057

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 8: Split ValueToId callers in js::Stringify. r=jandem!

Only call ValueToId for primitive values in preparation for the next patch
and instead call ToAtom for objects.

Depends on D83058

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1651732 - Part 9: Rename ValueToId to PrimitiveValueToId and assert its input is a primitive. r=jandem!

ValueToId turned out to be a pitfall we want to avoid, it looks quite similar
to ToPropertyKey, but is subtly different to that function.

Let's rename it to PrimitiveValueToId and assert its input is a primitive,
that should prevent any accidental use of that function when ToPropertyKey
should have been used instead.

Depends on D83059

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/291f6b508e95
Part 1: Change ProxyGetPropertyByValue to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a92e558e609a
Part 2: Change ProxySetPropertyByValue to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/59deaa77e027
Part 3: Change ProxyHas to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/f0857b845e88
Part 4: Change ProxyHasOwn to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d8c48aa96578
Part 5: Change IonGetPropSuperIC::update to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/23cb097cdecc
Part 6: Change DebuggerObject to use ToPropertyKey. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1612cbe2a958
Part 7: Change additional callers to use ToPropertyKey instead of ValueToId. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7c0eb528d744
Part 8: Split ValueToId callers in js::Stringify. r=jandem
https://hg.mozilla.org/integration/autoland/rev/0cdd4a7f1bb3
Part 9: Rename ValueToId to PrimitiveValueToId and assert its input is a primitive. r=jandem

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/291f6b508e95
https://hg.mozilla.org/mozilla-central/rev/a92e558e609a
https://hg.mozilla.org/mozilla-central/rev/59deaa77e027
https://hg.mozilla.org/mozilla-central/rev/f0857b845e88
https://hg.mozilla.org/mozilla-central/rev/d8c48aa96578
https://hg.mozilla.org/mozilla-central/rev/23cb097cdecc
https://hg.mozilla.org/mozilla-central/rev/1612cbe2a958
https://hg.mozilla.org/mozilla-central/rev/7c0eb528d744
https://hg.mozilla.org/mozilla-central/rev/0cdd4a7f1bb3