Implement structured clone for Map and Set

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: billm, Assigned: evilpie)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

According to the spec [1], we're supposed to handle this. It would be useful to have inside Firefox. We use structured cloning to pass messages between processes, and I'd like to be able to pass Maps.

[1] http://www.w3.org/html/wg/drafts/html/master/infrastructure.html#safe-passing-of-structured-data
Shumway wants to wants this so they can send Maps and Sets to remote processes for out-of-process SWF execution and rendering.
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [shumway:m3]
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Shumway wants to wants this

No, not really - I miscommunicated quite badly on this.

Shumway wants support for sending typed array buffers between processes, and I gave this as an example of other types of objects for which inter-process-passing support is being added.
Whiteboard: [shumway:m3]
Assignee: nobody → evilpies
Assignee: evilpies → nobody
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Posted patch WIP (obsolete) — Splinter Review
This adds the ability the clone Maps. I think I got cycles between map value/keys worked out as well. There is some stuff that is left to do: 1. Add back readId for backwards compat. 2. Make sure nothing else broke. 3. The MapObject methods don't fit our style. 4. Tests! :) 5. Sets
Posted patch WIP v2 (obsolete) — Splinter Review
This is basically done, what is left is just versioning and updating the indexdb.
Attachment #8454842 - Attachment is obsolete: true
Posted patch v1 (obsolete) — Splinter Review
Attachment #8454895 - Attachment is obsolete: true
Attachment #8454991 - Flags: review?(jorendorff)
Attachment #8454991 - Flags: review?(bent.mozilla)
Comment on attachment 8454991 [details] [diff] [review]
v1

Review of attachment 8454991 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1475,5 @@
> +UpgradeSchemaFrom15_0To16_0(mozIStorageConnection* aConnection)
> +{
> +  // The only change between 15 and 16 was a different structured
> +  // clone format, but it's backwards-compatible.
> +  nsresult rv = aConnection->SetSchemaVersion(MakeSchemaVersion(15, 0));

Copy paste error here, you can't leave it at 15.

Has this been tested?
Attachment #8454991 - Flags: review?(bent.mozilla) → review-
Oh dammit! It did actually look quite good on try: https://tbpl.mozilla.org/?tree=Try&rev=78b8f1dd7f54. (This was before I fixed the failing test)
Comment on attachment 8454991 [details] [diff] [review]
v1

Review of attachment 8454991 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch. I suppose we'd better do one more round on this one.

I reviewed everything except OpenDatabaseHelper.cpp.

Please also test that:
- expando properties added to a Map or Set are *not* cloned.
- a Map or Set is cloned correctly even if the user has modified its prototype chain.

::: js/src/vm/StructuredClone.cpp
@@ +915,5 @@
> +        if (JSID_IS_STRING(properties[i - 1]) || JSID_IS_INT(properties[i - 1])) {
> +        RootedValue val(context(), IdToValue(properties[i - 1]));
> +        if (!keys.append(val))
> +            return false;
> +        }

This code isn't indented properly.

But that's OK --- GetPropertyNames without JSITER_SYMBOLS will only return ids that are either STRING or INT, so you can MOZ_ASSERT that instead of using an if-block.

Note that the code immediately below does 'counts.append(properties.length())', so we had better actually push that many keys to the stack!

@@ +937,5 @@
> +        return false;
> +
> +    for (size_t i = newKeys.length(); i > 0; --i) {
> +        if (!keys.append(newKeys[i - 1]))
> +            return false;

Please push both keys and values to the stack here, to avoid redundant lookups later. I can see why you did it this way (following the existing code for objects), but it's OK to treat Maps and Sets a little differently.

The spec does not say what's supposed to happen if entries are added or removed by a getter triggered during step 6.3.1 or 6.3.2 or 7.3.1. Please file a bug against the HTML spec and see if you can get this addressed.

It might be easiest to make steps 6.1 and 7.1 specify that 'source' is a deep copy of the list, so that the algorithm is unaffected by mutation. Or, explicitly leave the behavior unspecified.

Please also file a bug against HTML noting that step 8 should cope with properties that have an ES6 symbols for the "property key" (what ES5 called the "property name"). Right now we just drop symbol-keyed properties; it's not clear what HTML *should* say about it.

@@ +996,2 @@
>      } else if (v.isNumber()) {
>          return out.writeDouble(v.toNumber());

Please change it to v.isDouble() and v.toDouble() if we're testing for int32 beforehand.

@@ +1176,2 @@
>                  bool found;
> +                if (!MapObject::has(context(), obj, key, &found))

This is one of the extra has lookups that can be avoided by pushing both keys and values to the stack; the MapObject::get below is the other one.

@@ +1188,5 @@
> +                }
> +            } else if (obj->is<SetObject>()) {
> +                bool found;
> +                if (!SetObject::has(context(), obj, key, &found))
> +                    return false;

Similarly, please skip this extra hash lookup.

@@ +1192,5 @@
> +                    return false;
> +
> +                if (found) {
> +                    if (!startWrite(key) ||
> +                        !startWrite(UndefinedValue()))

Please skip writing UndefinedValue() here.

@@ +1219,5 @@
> +                            !JSObject::getGeneric(context(), obj, obj, id, &val) ||
> +                            !startWrite(val))
> +                        {
> +                            return false;
> +                        }

Thanks for the extra curly braces.

@@ +1225,4 @@
>                  }
>              }
>          } else {
> +            out.writePair(SCTAG_END_OF_KEYS, 0);

Good catch!

@@ +1758,5 @@
> +            if (!MapObject::set(context(), obj, key, val))
> +                return false;
> +        } else if (obj->is<SetObject>()) {
> +            if (!SetObject::add(context(), obj, key))
> +                return false;

It's worth rearranging the logic a little so that when obj is a SetObject we don't read two values. The whole point of Set is to be more compact than Map...
Attachment #8454991 - Flags: review?(jorendorff)
Posted patch v2Splinter Review
Attachment #8454991 - Attachment is obsolete: true
Attachment #8458305 - Flags: review?(jorendorff)
Attachment #8458305 - Flags: review?(bent.mozilla)
Comment on attachment 8458305 [details] [diff] [review]
v2

Review of attachment 8458305 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Again, I reviewed everything except OpenDatabaseHelper.cpp.

Please add just one more test: a cycle through a key of a Map is handled correctly. Like this:
    var m = new Map();    m.set(m, 1);

::: js/src/jit-test/tests/structured-clone/Map.js
@@ +69,5 @@
> +obj = [...magic.keys()][0];
> +assertEq(obj.a, 1);
> +assertEq(magic.get(obj), "b");
> +
> +// Make sure expandos aren't cloned

Please file a followup bug in Bugzilla about also cloning expando properties and cite it here. You can link it to the HTML bug you filed. If Hixie WONTFIXes that, we should have something on file. Thanks.
Attachment #8458305 - Flags: review?(jorendorff) → review+
Comment on attachment 8458305 [details] [diff] [review]
v2

Review of attachment 8458305 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on dom/indexedDB changes
Attachment #8458305 - Flags: review?(bent.mozilla) → review+
Blocks: 1041172
https://hg.mozilla.org/mozilla-central/rev/225fa7edfb16
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.