1441141 - postMessage() exception order

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Anne (:annevk)]

Per the specification we should check transferList errors before serializing. https://github.com/w3c/web-platform-tests/pull/9672 has tests.

Comment 1

[Anne (:annevk)]

This is changing a bit in https://github.com/whatwg/html/pull/3557 and I'm also working on updating the tests.

Comment 2

[Anne (:annevk)]

Tests are updated now and available at http://w3c-test.org/html/infrastructure/safe-passing-of-structured-data/transfer-errors.window.html.

This should be fairly trivial to fix Andrea.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - DOM side

This part makes the test happy from a DOM side point of view. ArrayBuffers are handled by the JS engine. I'm going to submit a separate patch for that part.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8960913 [details] [diff] [review]
part 1 - DOM side

Hopefully wpt catches all these different cases

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - JS + DOM

Comment 7

[Andrea Marchesini [:baku]]

These patches fix this test too: https://github.com/w3c/web-platform-tests/pull/10123

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8961007 [details] [diff] [review]
part 2 - JS + DOM

>+StructuredCloneHolder::CustomCanTransferHandler(JSContext* aCx,
>+                                                JS::Handle<JSObject*> aObj)
>+{
>+  if (!mSupportsTransferring) {
>+    return false;
>+  }
>+
>+  JS::Rooted<JSObject*> obj(aCx, aObj);
>+
>+  {
>+    MessagePort* port = nullptr;
>+    nsresult rv = UNWRAP_OBJECT(MessagePort, &obj, port);
>+    if (NS_SUCCEEDED(rv)) {
>+      return true;
>+    }
>+
>+    if (mStructuredCloneScope == StructuredCloneScope::SameProcessSameThread ||
>+        mStructuredCloneScope == StructuredCloneScope::SameProcessDifferentThread) {
>+      OffscreenCanvas* canvas = nullptr;
>+      rv = UNWRAP_OBJECT(OffscreenCanvas, &obj, canvas);
>+      if (NS_SUCCEEDED(rv)) {
>+        return true;
>+      }
>+
>+      ImageBitmap* bitmap = nullptr;
>+      rv = UNWRAP_OBJECT(ImageBitmap, &obj, bitmap);
>+      if (NS_SUCCEEDED(rv)) {
>+        return true;
>+      }
>+    }
>+  }
It is rather annoying that we have this MessagePort/OffscreenCanvas/ImageBitmap check now at least in 3 places, but I don't have better ideas.
But could you at least document somewhere what all needs to be updated when adding support for new transferable DOM objects.

> MessagePort::CloneAndDisentangle(MessagePortIdentifier& aIdentifier)
> {
>   MOZ_ASSERT(mIdentifier);
> 
>+  if (mHasBeenTransferredOrClosed) {
>+    return false;
>+  }
>+
>+  mHasBeenTransferredOrClosed = true;
Why we need this? Shouldn't mState be enough to cover this?
Or add some new state?
Adding a boolean makes the object kind of have two states, mState + mHasBeenTransferredOrClosed, and that is 
rather confusing.
But please explain, and fix or ask review again.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8961007 [details] [diff] [review]
part 2 - JS + DOM

> >+  mHasBeenTransferredOrClosed = true;

This is needed because we want to deny the transferring of a port only when it has been already transferred or when it has manually been closed.
mState is about the state machine of MessagePort. When port1 is closed, port2 is sync or async closed as well. But that port can still be transferred by spec. mHasBeenTransferredOrClosed is used to know if transferring or manual close() happen.

I'm going to add this comment:

// mHasBeenTransferredOrClosed is used to know if this port has been manually closed or transferred via postMessage. Note that if the entangled port is closed, this port is closed as well (see mState) but, just because close() has not been called directly, by spec, this port can still be transferred.

Comment 10

[Andrea Marchesini [:baku]]

> MessagePort/OffscreenCanvas/ImageBitmap check now at least in 3 places, but
> I don't have better ideas.
> But could you at least document somewhere what all needs to be updated when
> adding support for new transferable DOM objects.

Would be nice if we move part of this logic directly in webIDL bindings introducing [serializable/transferable] keywords.

Comment 11

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

Comment on attachment 8961007 [details] [diff] [review]
part 2 - JS + DOM

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

::: js/src/vm/StructuredClone.cpp
@@ +1699,5 @@
>              JSAutoCompartment ac(cx, arrayBuffer);
> +
> +            if (arrayBuffer->isDetached())
> +                return false;
> +

This needs to report an error if it's returning false. I think

        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_DETACHED);

would do it. (Ugh, should rename that to JSMSG_ARRAY_BUFFER_DETACHED; the actual message only mentions ArrayBuffer.)

This appears to be a bug in the current implementation. You could clone while transferring an ArrayBuffer, then detach that ArrayBuffer during cloning. If you're DifferentProcess, you'll end up with an empty ArrayBuffer with no error. Otherwise, you'll get an uncatchable exception.

This can't be right. We have tests for this.... oh.

            // The throw is not yet implemented, bug 919259.
            //var copy = deserialize(serialize([ old, mutator ], [old]));

I'm looking into that now, trying to understand the full issue here and whether we're making it worse. It appears that this spec fix is related to the older bug 919259 spec bug. I'll r+ this patch because it's not making anything worse.

But at the very least: this cannot just return false without setting an exception. That would throw an uncatchable exception. It's a little to figure out, because there are two pre-existing problems of this sort in this code already. The main one is

1710:                    return false; // already transferred data

I'll fix them in another bug, I guess. (This one is pretty easy to fix here, but I'm not sure the other one is quite as straightforward, and I need to add a test along with this one.)

Comment 12

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

Comment on attachment 8961007 [details] [diff] [review]
part 2 - JS + DOM

Oops, I think I overwrote smaug's r+ somehow.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: js.patch

Steve, I need you to take a look at this patch again. There is 1 only 1 diff: I have to unwrap the tObj in order to have positive checks in is<ArrayBuffer> is<SomethingElse>.

I hope this is correct.

Comment 14

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

Comment on attachment 8962016 [details] [diff] [review]
js.patch

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

::: js/src/vm/StructuredClone.cpp
@@ +1102,5 @@
>              return reportDataCloneError(JS_SCERR_TRANSFERABLE);
>          tObj = &v.toObject();
>  
> +        RootedObject unwrappedObj(cx, CheckedUnwrap(tObj));
> +

I'm surprised we haven't been bitten by this already!

This should work, except (1) you should check the result of CheckedUnwrap and ReportAccessDenied(cx) if it fails; (2) you should use JSAutoCompartment ac(cx, unwrappedObj) so that we call canTransfer() from the matching compartment; and (3) the wrapped tObj should be stored in transferableObjects, not the unwrapped one, as otherwise it would create a cross-compartment edge and if we happened to GC one side and not the other, Bad Stuff would happen.

Hopefully it will still work with those changes. I'll look at creating a test case for the existing code.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - JS + DOM

Comment 16

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

Comment on attachment 8962651 [details] [diff] [review]
part 2 - JS + DOM

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

::: js/src/vm/StructuredClone.cpp
@@ +1133,5 @@
> +                return reportDataCloneError(JS_SCERR_TRANSFERABLE);
> +
> +            JSAutoCompartment ac(cx, unwrappedObj);
> +            if (!callbacks->canTransfer(cx, unwrappedObj, closure))
> +                return false;

I guess this is fine.

I was imagining the JSAutoCompartment to happen right after the unwrapping, so that all of the type handling would be within the target compartment. But conceptually the transferableObjects lookup/insert is in the outer compartment, so that would require a separate scope... ok, what you have is fine.

If we ever touch this code again, I think the series of if statements should probably be lifted into a separate function that does a JSAutoCompartment as its first statement.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa314a4f050
MessagePort cannot be cloned/transferred if disentanged, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9890a6738737
Update the StructuredCloneAlgorithm to follow the latest version of the spec, r=smaug, r=sfink

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0fa314a4f050
https://hg.mozilla.org/mozilla-central/rev/9890a6738737

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10309 for changes under testing/web-platform/tests