1433291 - JSStructuredCloneReader reenters interpreter to convert array to property id (causing long runtime)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Don't allow serialized data to use objects as property keys

Comment 2

[Jason Orendorff [:jorendorff]]

Christian, does this help?

Comment 3

[Christian Holler (:decoder)]

Confirmed, this solves the problem for me :) Thanks!

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8945902 [details] [diff] [review]
Don't allow serialized data to use objects as property keys

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

Oh! That's amusing.

It does make me wonder why this seems reasonably fast:

var proto = {};
for (var i = 0; i < 1000000; i++) {
  o = Object.create(proto);
  o[o] = i;
}

How many properties is this test case trying to set?

::: js/src/vm/StructuredClone.cpp
@@ +2680,5 @@
> +                JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr,
> +                                          JSMSG_SC_BAD_SERIALIZED_DATA,
> +                                          "property key expected");
> +                return false;
> +            }

I don't follow the isSymbol case. It appears that when you serialize, Symbols are TypeErrors if you serialize them directly, or are silently ignored if they are property keys. I attempted to determine this from the spec, but the spec makes no sense to me: it says to go through each key in value.[[GetOwnPropertyKeys]]() and do something with every key that is of type String. But in the es spec, GetOwnPropertyKeys (well, OrdinaryOwnPropertyKeys) just returns the property keys, so by that I would say that we should be dropping all integer-keyed properties. If I ignore the String type check, then it seems like the Symbol key should get serialized, which is not a thing that can be done. So it does appear that the String check is intended to drop Symbols.

So if you read in a Symbol, shouldn't that also be an error? Based on the expected and observed behavior of Symbol keys getting dropped on the floor, even if Enumerable. This code seems to happily accept the Symbol as a key. Which is not the worst thing in the world since it appears impossible in practice (reading in the key will never produce a Symbol, no matter how much you've hacked the binary data), but still.

Comment 5

[Jason Orendorff [:jorendorff]]

This isn't sec-anything. Revising my patch...

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> Oh! That's amusing.
> 
> It does make me wonder why this seems reasonably fast:
> 
> var proto = {};
> for (var i = 0; i < 1000000; i++) {
>   o = Object.create(proto);
>   o[o] = i;
> }

The test case isn't creating lots of properties; it's stringifying `Array(415153856)`. The properties don't exist.

Comment 7

[Jason Orendorff [:jorendorff]]

...as it clearly says right here (points at lines and lines of meaningless numbers in test case)

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> (In reply to Steve Fink [:sfink] [:s:] from comment #4)
> > Oh! That's amusing.
> > 
> > It does make me wonder why this seems reasonably fast:
> > 
> > var proto = {};
> > for (var i = 0; i < 1000000; i++) {
> >   o = Object.create(proto);
> >   o[o] = i;
> > }
> 
> The test case isn't creating lots of properties; it's stringifying
> `Array(415153856)`. The properties don't exist.

Oh! This is slow because it's stringifying a huge Array. I thought it was a fast stringification run many many times, as in my test case that produces the property name "[object Object]" over and over.

Ok, that makes sense. I can accept that generating a string with 415 million commas might take a while. I'm somewhat curious what horrific Rope-flattening shenanigans it might be doing here, but not enough to find out.

Comment 9

[Steve Fink [:sfink] [:s:]]

(and my initial assumption was dumb; why would a small structured clone buffer do lots of stringifications? I guess I was thrown off by the "reentering the interpreter thing", which I immediately assumed meant we were doing tons of JS <-> C++ crossings.)

I just had to check this a little bit. It looks like stringifying a huge empty array does a GetArrayElement->NativeGetProperty on each index. Or at least, those are the top 2 functions in |perf report|. We could make stringifying lots of array holes massively faster! Or not.

Comment 10

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/663a36bf36a34363e9e934abe63c586c8c370725
Bug 1433291 - Don't allow serialized data to use objects as property keys. r=sfink.

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/663a36bf36a3