ref. mozilla#780 fix Object.assign() when there's a prototype

[dcitron]

Fix Object.assign() -- and [[Set]] in general -- for the case where the target object inherits from a prototype that already contains the property being set

Closes #780, #1195

[rbri]

Looks good to me - many thanks.

Have tested your fix with HtmlUnit also and all test passing.

Test262SuiteTest also passes as expected without no new tests passing - from my point of view this is also expected because the issue was also not detected by this suite.

[gbrail]

I'm sorry that I haven't touched this yet. I have actually looked at it a few times and I'm worried about introducing such a specific test so deep in the "putImpl" method, which is a very hot code path, and one that is already complicated enough. The abstract thing that that particular method already does is pretty hard to define and I fear that this makes it harder.

Is there a change that we could make either in the Object.assign code, or in the "AbstractEcmaOperations" class that would be still in keeping with the spec but which would not cause us to have to change something so fundamental. I don't know who might be depending on the existing behavior but given how many things depend on Rhino I fear that it might be a lot of things.

[dcitron]

It's a fair point, @gbrail -- I'm perhaps even more concerned now that you, a primary author on this project, are concerned -- I'm far from an expert on this codebase, and I arrived at this change after writing those NativeObjectTest validations and walking through the code with a debugger ...

I can look at it again from a different perspective, as you suggest. I'm sure you read it already, but I feel like I described what I believe to be the bug to the best of my ability at #780 (comment)

I don't know, @tuchida, if you're able to look at AbstractEcmaObjectOperations.put() again to see if it's possible to fix this there. I started out in that direction (my first breakpoints were in there), but I couldn't reason my way to a solution in those functions. For one thing, the Slot is not available at that level, and slots are not exposed in that way out of ScriptableObject.

I suppose one way to fix this in AbstractEcmaObjectOperations.put() would be to add a boolean getter to ScriptableObject to allow those Slot checks to happen from outside the object ... something like this:

class AbstractEcmaObjectOperations {
    // ...
    static void put(Context cx, Scriptable o, String p, Object v, boolean isThrow) {
        Scriptable base = ScriptableObject.getBase(o, p);
        if (base == null) base = o;

        if (base instanceof ScriptableObject) {
            ScriptableObject baseObject = (ScriptableObject) base;
            // -- START OF NEW CODE --
            if (o instanceof ScriptableObject
                    && baseObject != o
                    // -- NEW FUNCTION in ScriptableObject
                    //    that gets the slot and checks if
                    //    it's not null, not read-only, and
                    //    a value slot --
                    && baseObject.isWriteableValueSlot(p, 0, isThrow)) {
                // If base is not the Receiver (start) object
                // and if the slot is a Writeable data descriptor
                // then invoke putImpl() on the Receiver (start) object
                ((ScriptableObject) o).putImpl(p, 0, o, v, isThrow);
            } else {
                // -- END OF NEW CODE --
                baseObject.putImpl(p, 0, o, v, isThrow);
            }
        } else {
            base.put(p, o, v);
        }
    }
    // ...
}

Would that be a route worth pursuing, @gbrail and @tuchida ? It would have to be done in both AbstractEcmaObjectOperations.put() methods and would involve a slightly goofy new method ScriptableObject.isWriteableValueSlot() as shown above, but if you both think this is the better route, I can work this one up and see if it passes all the same unit tests.

[p-bakker]

While I understand @gbrail concerns, I wonder if the issue we're trying to solve here is limited to scenario's where Object.assign is used (meaning whether it makes sense to see if a fix could be tied exclusively to the Object.assign impl.) or whether there are tests that can be written that trigger the issue while not using Object.assign

As for putting it inside the AbstractEcmaObjectOperations.put() implementation: besides that method then not following the spec closely, IMHO it would make the overall O.[[SET]](P,V,O) implementation even harder to readon with, with it being split out over multiple methods.

All that said: I do understand the concerns about making changes to something so deep in the engine, so we need to be carefull about it, but IMHO if its the best place to do it, we just have to bite the bullit...

[gbrail]

Sorry I'm making this complicated, but that's a really common place.

What if we were to implement an "AbstractEcmaOperations.set" method that closely followed the "[[Set]]" specification to the letter. Would that fix Object.assign?

Otherwise, I'd be more comfortable fixing this in Object.assign until we understand better what our options might be.

[dcitron]

Should I work on my proposed solution from #1242 (comment) then?

[rbri]

Hopefully i have some time on this weekend to have a look at all the Rhino stuff in the queue - sorry

[rbri]

As always i agree with @gbrail concerns but this time it's the other way around - this issue is about something that was broken by the fix/improvement we made. We did not notice that but at least the HtmlUnit test suite shows that we broke the existing behavior. So this fix is about bringing back the old behavior....

[p-bakker]

Did some looking: this works as expected:

var targetProto = { a: 1, b: 2 };
var target = Object.create(targetProto);
target.b = 4;
JSON.stringify(target); // yields {"b":4}

And this doesn't:

var targetProto = { a: 1, b: 2 };
var target = Object.create(targetProto);
var source = { b: 4};
var assigned = Object.assign(target, source);
JSON.stringify(assigned); // yields {}

Both Object.assign(target, source); and target.b = 4; eventually go through ScriptableObject.putImpl, but before getting there follow different paths:

• Object.assign: NativeObject.execIdCall.ConstructorId_assign > AbstractEcmaObjectOperations.put > ScriptableObject.putImpl
• target.b = 4;: ScriptableObject.putProperty > IdScriptableObject.put > ScriptableObject.put > ScriptableObject.putImpl

And I think the key difference is that when doing Object.assign, the logic that is in ScriptableObject.put (that checks the return value of putImpl and if false calls start.put) and that would get called when doing target.b = 4 is plain missing

I've locally quickly added code that is missed and ran the test and with the changes in place it yields the correct result:

    static void put(Context cx, Scriptable o, String p, Object v, boolean isThrow) {
        Scriptable base = ScriptableObject.getBase(o, p);
        if (base == null) base = o;

        if (base instanceof ScriptableObject) {
            if (((ScriptableObject) base).putImpl(p, 0, o, v, isThrow)) return;

            o.put(p, o, v);
        } else {
            base.put(p, o, v);
        }
    }

Whether this is the correct and a complete fix I don't know, just quickly analysed the codepaths and ran a quick test

BTW: looking at the AbstractEcmaObjectOperations.put method, I also wonder if the base.put(p, o, v); call if base not an instance of ScriptableObject works correctly as well: will the property every get set on o?

[rbri]

because @p-bakker solution is now merged (#1294) i like to close this on.
Many thanks to all working on this.