Skip to content

Structured cloning for new ES types and for IDL-defined objects. #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
domenic opened this issue Feb 29, 2016 · 11 comments · Fixed by #783
Closed

Structured cloning for new ES types and for IDL-defined objects. #766

domenic opened this issue Feb 29, 2016 · 11 comments · Fixed by #783
Assignees
Labels
compat Standard is not web compatible or proprietary feature needs standardizing topic: serialize and transfer

Comments

@domenic
Copy link
Member

domenic commented Feb 29, 2016

https://jsbin.com/ququca/edit?html,output

The current spec says to basically create new empty objects (copying over any own properties). Chrome does this, except for IDL-defined objects, for which it throws a DataCloneError.

Edge and Firefox instead throw DataCloneErrors. (Or a TypeError for argsIterator in Firefox, presumably because they haven't implemented arguments's iterator.)

I think we should define that the default [[Clone]] method for IDL-defined objects is to throw a DataCloneError? And tweak the definition of cloneable IDL objects a bit.

For the ES types, I see a few solutions:

  • Any object with internal slots that we don't otherwise handle throws a DataCloneError. This can remove some existing steps and future-proofs us against new ES types.
  • Specifically black-list all the recently-added types, like we do with e.g. errors or functions.
  • Keep the spec with Chrome's behavior of producing an empty object and copying over own properties.
@domenic domenic added the compat Standard is not web compatible or proprietary feature needs standardizing label Feb 29, 2016
@domenic
Copy link
Member Author

domenic commented Feb 29, 2016

Added a proxy test case. Firefox and Edge again throw a DataCloneError, and this time Chrome outputs an empty object, ignoring the own property. The spec currently requires the DataCloneError per:

Otherwise, if input is an exotic object, then throw a DataCloneError exception.

@annevk
Copy link
Member

annevk commented Mar 1, 2016

Should we keep [[Transfer]] inconsistent and only exposed on objects annotated as transferable objects? (This is why I initially wanted to give all objects these internal methods. Implications would be more obvious and new objects would automatically throw unless specified otherwise.)

@domenic
Copy link
Member Author

domenic commented Mar 1, 2016

I don't think there's a need to be consistent there. Transfer doesn't have a fallback case, so it's very different from cloning in terms of who needs to define what. If we gave all objects these internal methods, that wouldn't change things either; the fallback case would still apply to new objects, unless they were defined to explicitly override the default own-properties [[Clone]].

@annevk
Copy link
Member

annevk commented Mar 1, 2016

That was the idea. By default [[Clone]] would throw and each object that wants to partake defines its own.

@domenic
Copy link
Member Author

domenic commented Mar 1, 2016

That can't work, because that will break the own-properties cloning of ordinary objects.

@annevk
Copy link
Member

annevk commented Mar 1, 2016

Good point.

Any object with internal slots that we don't otherwise handle throws a DataCloneError. This can remove some existing steps and future-proofs us against new ES types.

How would you word this? My thought was to just block them explicitly.

@domenic
Copy link
Member Author

domenic commented Mar 1, 2016

I would probably literally say "Otherwise, if input has any internal slots, then throw a DataCloneError exception." You could then remove step 17 for [[ErrorData]]. I guess this would go after the current step 19.

@domenic
Copy link
Member Author

domenic commented Mar 1, 2016

It seems like some sort of implementation strategy mismatch TBH.

I'd guess Edge and Firefox are checking if the object has any private data stored on it (using implementation-specific mechanisms for doing that), and if so throwing a DataCloneError. So that goes with the "anything with internal slots" approach. Whereas Chrome is explicitly blocklisting certain things (like Errors), corresponding to the explicit list of blocklisted internal slots.

Maybe @ajklein can comment on whether the "has any internal slots" approach is feasible for Chrome.

@domenic
Copy link
Member Author

domenic commented Mar 1, 2016

For the record, @annevk and I spelunked through Firefox's implementation a bit. Key lines are:

This notion of "plain object" isn't something terribly well supported by the ES spec, but maybe "has no internal slots" is a good proxy for it. (In ES5 it was taken care of by [[Class]] being Object.)

@annevk
Copy link
Member

annevk commented Mar 1, 2016

Paging @ajklein for opinions. Our plan here is to align with Edge and Firefox.

@ajklein
Copy link
Contributor

ajklein commented Mar 1, 2016

Not sure what you mean by "feasible". Any choice here should be implementable. Mostly Chrome's behavior can be explained by the fact that the structured clone code is in Blink and the new ES object code is in V8. So it's really a matter of what we'd like to say in the spec. From that point of view, I guess it's more concise to say "has internal slots" (though I don't know how that interacts with, say, the private state proposal).

annevk added a commit that referenced this issue Mar 2, 2016
StructuredClone used to throw for “host objects” (now objects with a [[Clone]] internal method) and
objects whose [[Class]] was not “Object” (now objects which have an internal slot other than
[[Prototype]] or [[Extensible]]). This commit restores that behavior.

This regressed in bfb960c.

This fixes #766.

This  affects Chrome. Edge and Firefox already do the right thing.
annevk added a commit that referenced this issue Mar 3, 2016
StructuredClone used to throw for “host objects” (now objects with a [[Clone]] internal method) and
objects whose [[Class]] was not “Object” (now objects which have an internal slot other than
[[Prototype]] or [[Extensible]]). This commit restores that behavior.

This regressed in bfb960c.

This fixes #766.

This  affects Chrome. Edge and Firefox already do the right thing.
annevk added a commit that referenced this issue Mar 3, 2016
StructuredClone used to throw for “host objects” (now objects with a [[Clone]] internal method) and
objects whose [[Class]] was not “Object” (now objects which have an internal slot other than
[[Prototype]] or [[Extensible]]). This commit restores that behavior.

This also fixes the invocation of [[Clone]] by passing memory too.

This regressed in bfb960c.

This fixes #766.

This  affects Chrome. Edge and Firefox already do the right thing.
annevk added a commit that referenced this issue Mar 3, 2016
StructuredClone used to throw for “host objects” (now objects with a [[Clone]] internal method) and
objects whose [[Class]] was not “Object” (now objects which have an internal slot other than
[[Prototype]] or [[Extensible]]). This commit restores that behavior.

This also fixes the invocation of [[Clone]] by passing memory too.

This regressed in bfb960c.

This fixes #766.

This  affects Chrome. Edge and Firefox already do the right thing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat Standard is not web compatible or proprietary feature needs standardizing topic: serialize and transfer
Development

Successfully merging a pull request may close this issue.

3 participants