Bug 202711 – Post increment/decrement should only call ToNumber once

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

Currently they call it twice, see the following test case:
```
var o = {};
var counter = 0;
o.valueOf = () => {counter ++; return 42};
o++;
if (counter != 1)
    throw "valueOf was executed " + counter + " times during a post-increment instead of once!";
```

As far as I can tell this is not spec-compliant:
12.4.4 Postfix Increment Operator requires a single call to ToNumeric.
Then there is a single chain of ToNumeric -> ToNumber -> ToPrimitive -> OrdinaryToPrimitive -> valueOf with no reason for the call to be duplicated anywhere.

The problem appears to be from:
```
static RegisterID* emitPostIncOrDec(BytecodeGenerator& generator, RegisterID* dst, RegisterID* srcDst, Operator oper)
{
    if (dst == srcDst)
        return generator.emitToNumber(generator.finalDestination(dst), srcDst);
    RefPtr<RegisterID> tmp = generator.emitToNumber(generator.tempDestination(dst), srcDst);
    emitIncOrDec(generator, srcDst, oper);
    return generator.move(dst, tmp.get());
}
```
which uses an emitToNumber, but then does an emitIncOrDec on the original value, which itself can lead to a slow path that does ToNumber anew.

[Robin Morisset]

Things are a bit more subtle than I expected: wrapping the testcase above in a function and calling that function suppresses the bug!
It comes back if "o++" is replaced by "var x = o++" so I suspect it is caused by the code:
```
RegisterID* PostfixNode::emitResolve(BytecodeGenerator& generator, RegisterID* dst)
{
    if (dst == generator.ignoredResult())
        return PrefixNode::emitResolve(generator, dst);
...
    RefPtr<RegisterID> oldValue = emitPostIncOrDec(generator, generator.finalDestination(dst), value.get(), m_operator);
...
}
```
also in NodesCodegen.cpp. It visibly replaces "o++;" by "++o;" when the result is not stored anywhere.

The mystery is why it only occurs when wrapped in a function, and not at the top-level.

[Robin Morisset]

Created attachment 380545 [details]
Patch

[Mark Lam]

Comment on attachment 380545 [details]
Patch

r=me

[EWS Watchlist]

Comment on attachment 380545 [details]
Patch

Attachment 380545 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13113521

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 380608 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Keith Miller]

Seems like this patch has broken jsc EWS... :(

[Alexey Proskuryakov]

*** Bug 202710 has been marked as a duplicate of this bug. ***

[Robin Morisset]

Created attachment 380943 [details]
Patch

[EWS Watchlist]

Comment on attachment 380943 [details]
Patch

Attachment 380943 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13131267

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla

[Robin Morisset]

Created attachment 381002 [details]
Patch

Same patch as before, to check that the previous test failure was a flake.

[Saam Barati]

Comment on attachment 381002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381002&action=review

r=me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1791
> +    RefPtr<RegisterID> tmp = generator.emitToNumber(generator.newTemporary(), srcDst);
> +    RefPtr<RegisterID> result = generator.tempDestination(srcDst);
> +    generator.move(result.get(), tmp.get());
> +    emitIncOrDec(generator, result.get(), oper);
> +    generator.move(srcDst, result.get());

why not something like this?
```
RefPtr<RegisterID> result = generator.emitToNumber(generator.tempDestination(dst), srcDst);
emitIncOrDec(generator, result.get(), oper);
generator.move(srcDst, result.get());
return generator.move(dst, result.get());
```

[Robin Morisset]

Comment on attachment 381002 [details]
Patch

The jsc-mips bot appears stuck, but since it does not show an actual test failure and all other bots are green, I'm trying to land this.

[WebKit Commit Bot]

Comment on attachment 381002 [details]
Patch

Clearing flags on attachment: 381002

Committed r251371: <https://trac.webkit.org/changeset/251371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56471606>