Update previous versions to v8 to 11.3.244.4 (or apply one line fix)

[bmacnaughton]

Version

v18.16.0

Platform

Linux uxul 5.19.0-38-generic #39~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Mar 17 21:16:15 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

v8

What steps will reproduce the bug?

I cannot reproduce without a 3rd party dependency because this requires creating a string with external data. I'm using @contrast/distringuish to do that. It's a small C++ add-on that associates persistent data with an externalized string.

const d = require('@contrast/distringuish');

const obj = {thing: 'some string'};

const thing2 = d.externalize('some string');
Object.defineProperty(obj, 'thing', {value: thing2});

console.log(d.isExternal(obj.thing)); // false, should be true

How often does it reproduce? Is there a required condition?

It always reproduces. The required condition is that the two strings are equal from a JavaScript standpoint.

What is the expected behavior? Why is that the expected behavior?

true

The string should be externalized because the non-externalized string in the object should have been replaced by the externalized string.

What do you see instead?

false

Additional information

This is really a v8 bug and was fixed in v20.0.0 by deps: update V8 to 11.3.244.4.

This is a request to backport either the new version of v8 or to apply a one-line fix. The one line fix would be to replace

      (!desc->has_value() ||
       (current->has_value() && current->value()->SameValue(*desc->value()))) &&

with

    !desc->has_value() &&

at line 1496 in deps/v8/src/objects/js-objects.cc

[tniessen]

cc @targos

[targos]

Backporting a new version of V8 is not possible due to breaking changes in the native API.
Can you point to the original V8 CL that did this one line fix?

[bmacnaughton]

@targos - good question. i hadn't done all the homework i should have.

The commit is v8/v8@5425636. And it is quite a bit more than one line.

I made a one line change (well, two lines to one), rebuilt node, and all tests passed. I had not gone back and looked at the original commit. And I didn't run any performance measurements.

my change was to deps/v8/src/objects/js-objects.cc in the function Maybe<bool> JSReceiver::ValidateAndApplyPropertyDescriptor for case // 3.

from:

      (!desc->has_value() ||
       (current->has_value() && current->value()->SameValue(*desc->value()))) &&

to (the same as the code in v8 11..3.244.4 in node v20.0.0):

!desc->has_value() &&

There's more to this than I initially identified. I'll continue working on a better understanding., but it turns out that 11.3.244.4 doesn't fix the issue even with that change - it just moves the problem because it's still using SameValue() in other places. Feel free to close - if it's not fixed in v8 then this is still a v8 issue and i have more work to do.

[bmacnaughton]

Apologies for the false starts/stop. This was fixed in 20.0.0 by the code change above. It passes all tests when I make the change above in 16.19.1. But I cannot speak to what extent this change is interrelated with work in the code-gen and execution engines.

Unless you're comfortable with changing the code as shown above (I am not), I think we can close this. The change definitely works and passes all node tests, but that doesn't speak to possible performance impact.

[targos]

@verwaest Do you think it's safe to apply the one-line change from v8/v8@5425636#diff-65a0747b25b4f5334d148b3a59b3fce19f027b9213dcda1ed1f2672c963f8704L1535-R1535 in an older V8 version (10.2 in that case)?

[targos]

It seems safe to me (removing an optimization, not changing observable behavior)