Object.assign is broken after 1.7.15 with custom ScriptableObjects

[pavelhoral]

The issue

This is related to #780, which changed how Object.assign works. That change makes sense as it aligns the implementation with the ECMA specification. However it is a breaking change for all custom ScriptableObject based classes (not only inside RhinoJS itself, but in depending projects as well).

Symptoms

Classes extending ScriptableObject quite often only override basic get / put / has / getIds methods. Trying to use Object.assign ends with error due to missing attribute slot:

• rhino/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

  Line 486 in cc302b4

  public int getAttributes(String name) {

• rhino/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

  Line 2598 in cc302b4

  throw Context.reportRuntimeErrorById("msg.prop.not.found", str);

js> var customNative = new Packages.org.example.javascript.CustomNative()
js> Object.assign({}, customNative)
js: Property hello not found.
        at (unknown):18

Quick solution to the issue

The first solution that comes to my mind is to make sure that missing attribute slot does not end with exception... at least not in case when called from Object.assign as the specification clearly states the following:

So undefined property attributes is a well defined state... the property should be skipped.

Proper solution

Previous quick solution still means 1.7.15+ brought breaking changes for custom ScriptableObject implementations. I would appreciate if there would be clear example how custom NativeObject / IdScriptableObject / ScriptableObject should be implemented... in the case of IdScriptableObject implementation I guess one needs to override findInstanceIdInfo method?

Footnote

Not sure if this is a partial duplicate of #1549. That issue's description is a bit cryptic for me.

[p-bakker]

Added the docs tag as well, as in the end, we'll need to update the docs for implementing custom host objects as well: currently all examples are about implementing the Scriptable interface, but while that works, it doesn't result in host objects that extend Object, which is a best practice nowadays

[pavelhoral]

I was playing with this a bit in a project that is using RhinoJS and I am not able to fix it on my end as NativeObject's assign method calls package private method putImpl that I am not able to override:

rhino/rhino/src/main/java/org/mozilla/javascript/NativeObject.java

Line 674 in cbb0e2f

AbstractEcmaObjectOperations.put(

rhino/rhino/src/main/java/org/mozilla/javascript/AbstractEcmaObjectOperations.java

Line 210 in cbb0e2f

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

rhino/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

Line 2510 in cbb0e2f

boolean putImpl(Object key, int index, Scriptable start, Object value, boolean isThrow) {

The ScriptableObject#putImpl method is tightly coupled with the private slotMap property. This prevents subclasses to intercept such call (e.g. when the object represents custom java.util.Map wrappers).

[gbrail]

I would like to fix this before the releease. My current idea is:

1. Add try-catch in the "isEnumerable" method like @pavelhoral suggests
2. Somehow force Rhino into "strict mode" in Object.assign so that Object.assign behaves properly when not dealing with ScriptableObject subclasses.

[gbrail]

This is fixed by the PR, although it requires classes that override ScriptableObject to manage their own property access a bit differently -- take a look at the PR and the associated tests.