748309 - Typed Array structured clone doesn't do back-references

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Discovered while doing bug 743615. When we fix this, we should make sure that the todo in test_bug743615.html passes.

Comment 1

[Bobby Holley (:bholley)]

A little bit more detail:

If we have typed array foo, and do |bar = clone([foo, foo])|, we should have bar[0] === bar[1]. But the comparison currently fails.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

See also bug 675715.

Comment 3

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

Attachment: Structured clone should check for duplicates of all objects

Any reason why we can't just do what's in this patch? The spec seems to say that *any* object should get the same treatment.

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#safe-passing-of-structured-data

Comment 4

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/try/rev/7e065256b171 this patch breaks a couple of mochitests.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 658226 [details] [diff] [review]
Structured clone should check for duplicates of all objects

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

Tests are missing. Clearing r?; ping me when you post it with tests and I'll try to turn it around quickly.

::: js/src/jsclone.cpp
@@ +523,1 @@
>          AutoCompartment ac(context(), obj);

Switching compartments has to happen right after the Unwrap call, before startObject. We ought to have a test that flunks because of this somewhere.

Comment 6

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

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment on attachment 658226 [details] [diff] [review]
> Structured clone should check for duplicates of all objects
> 
> Review of attachment 658226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tests are missing. Clearing r?; ping me when you post it with tests and I'll
> try to turn it around quickly.

Oh, sorry. I should have marked this f? instead of r?. I already know this breaks existing tests; I just wanted to ask if there's some reason why this would be a bad idea before I beat it into working.

> ::: js/src/jsclone.cpp
> @@ +523,1 @@
> >          AutoCompartment ac(context(), obj);
> 
> Switching compartments has to happen right after the Unwrap call, before
> startObject. We ought to have a test that flunks because of this somewhere.

Very possible. Several tests fail with just this patch, though unfortunately they're all mochitests (see comment 4). I'll make some JS tests.

Comment 7

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

Attachment: Structured clone should check for duplicates of all objects

This seems to work, but I'm still working on updating the tests to the new semantics. I am uploading this version so baku can see it for bug 720083.

Comment 8

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

Attachment: Structured clone should check for duplicates of all objects

...and that last one I posted was buggy too. This one should be good, and passed Try: https://tbpl.mozilla.org/?tree=Try&rev=c9ca1253b0e344c69a6d30dc6c9ba59bb5da2fd9

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 661626 [details] [diff] [review]
Structured clone should check for duplicates of all objects

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

The tests are good but there doesn't seem to be a test of typed arrays!

Take a peek at clone-complex-object.js and see if it's duplicate code that should be commoned up, or if tests should be added there, whatever. I didn't look closely.

I'd love to see clone-object.js broken up into a few files, if you think that's worth doing. Just a thought.

r=me with minor testing comments addressed.

::: js/src/jsclone.cpp
@@ +841,5 @@
>          if (data >= allObjs.length()) {
>              JS_ReportErrorNumber(context(), js_GetErrorMessage, NULL,
>                                   JSMSG_SC_BAD_SERIALIZED_DATA,
> +                                 "invalid back reference in input");
> +            return false;

Er, yes. Return false. Thank you.

::: js/src/tests/js1_8_5/extensions/clone-object.js
@@ +16,5 @@
> +
> +// Set of properties on a cloned object that are legitimately non-writable,
> +// grouped by object type. The property name '0' stands in for any indexed
> +// property.
> +var non_writable = { 'String': [ 0 ] };

Maybe this didn't need to be *quite* so data-driven, but all right. :)

@@ +124,5 @@
> +                // Prevent infinite loops in the test due to cyclic data structures
> +                if (!seen.has(va)) {
> +                    queue.push([va, vb, path2]);
> +                    seen.add(va);
> +                }

If vb was previously seen, then we could assert that va matches the previously seen clone of vb.

If vb was not previously seen, then we could assert that va was not previously seen.

Just a thought.

@@ +305,5 @@
> +
> +    v = new ArrayBuffer(7);
> +    b = [ v, v ];
> +    a = check(b);
> +    assertEq(a[0] === a[1], true);

These could be commoned up better... just saying.

@@ +327,5 @@
> +    var objects = new getTestContent;
> +    try {
> +        while (true) {
> +            var obj = objects.next();
> +            print(obj);

The way to spell this sort of thing is:

    for (var obj of getTestContent()) {
        print(obj);
        check([obj]);
    }

Comment 10

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

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> Comment on attachment 661626 [details] [diff] [review]
> Structured clone should check for duplicates of all objects
> 
> Review of attachment 661626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests are good but there doesn't seem to be a test of typed arrays!

Other than the single Int8Array, you mean? Would you prefer a test of all of the different typed array subtypes?

I don't want to do a detailed test of the typed array cloning semantics, because they're totally wrong right now (bug 789593).

> Take a peek at clone-complex-object.js and see if it's duplicate code that
> should be commoned up, or if tests should be added there, whatever. I didn't
> look closely.

Yeah, I guess I should figure out the role of the different clone* tests.

> I'd love to see clone-object.js broken up into a few files, if you think
> that's worth doing. Just a thought.

I'll look at it.

> r=me with minor testing comments addressed.
> 
> ::: js/src/jsclone.cpp
> @@ +841,5 @@
> >          if (data >= allObjs.length()) {
> >              JS_ReportErrorNumber(context(), js_GetErrorMessage, NULL,
> >                                   JSMSG_SC_BAD_SERIALIZED_DATA,
> > +                                 "invalid back reference in input");
> > +            return false;
> 
> Er, yes. Return false. Thank you.

Oh, right. Shouldn't really be in this patch, but if you don't mind I'll leave it in.

> ::: js/src/tests/js1_8_5/extensions/clone-object.js
> @@ +16,5 @@
> > +
> > +// Set of properties on a cloned object that are legitimately non-writable,
> > +// grouped by object type. The property name '0' stands in for any indexed
> > +// property.
> > +var non_writable = { 'String': [ 0 ] };
> 
> Maybe this didn't need to be *quite* so data-driven, but all right. :)

Yeah, I was doing one failure at a time, and was anticipating way more exceptions than this. This way is harder to read but easier to modify; I guess I'll leave it even though if I were starting from scratch I wouldn't bother.

> @@ +124,5 @@
> > +                // Prevent infinite loops in the test due to cyclic data structures
> > +                if (!seen.has(va)) {
> > +                    queue.push([va, vb, path2]);
> > +                    seen.add(va);
> > +                }
> 
> If vb was previously seen, then we could assert that va matches the
> previously seen clone of vb.
> 
> If vb was not previously seen, then we could assert that va was not
> previously seen.
> 
> Just a thought.

Ok, easy enough.

> @@ +305,5 @@
> > +
> > +    v = new ArrayBuffer(7);
> > +    b = [ v, v ];
> > +    a = check(b);
> > +    assertEq(a[0] === a[1], true);
> 
> These could be commoned up better... just saying.

Yeah, but then I couldn't use the line number to figure out which one was failing. When do I get an easy-to-use command-line shell debugger so it doesn't matter anymore? :-)

> @@ +327,5 @@
> > +    var objects = new getTestContent;
> > +    try {
> > +        while (true) {
> > +            var obj = objects.next();
> > +            print(obj);
> 
> The way to spell this sort of thing is:
> 
>     for (var obj of getTestContent()) {
>         print(obj);
>         check([obj]);
>     }

Cool, thanks.

Comment 11

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

I moved all test changes from clone-object.js to clone-complex-object.js because it already had proper testing of the identity of recurring objects.

http://hg.mozilla.org/integration/mozilla-inbound/rev/2c9976725a57
http://hg.mozilla.org/integration/mozilla-inbound/rev/f0e182ab06a9

Comment 12

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

https://tbpl.mozilla.org/?tree=Try&rev=bdc45a1e022f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0e182ab06a9
https://hg.mozilla.org/mozilla-central/rev/2c9976725a57