Skip to content
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

HTML: test MessageChannel and SharedArrayBuffer more #19572

Merged
merged 2 commits into from
Oct 28, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Oct 8, 2019

@yutakahirano @dtapuska @domenic @chihweitung @asutherland the final test in what is now window-iframe-messagechannel.https.html in this PR is the most interesting and Chrome fails there, despite @binji claiming in whatwg/html#4209 (comment) that it works in Chrome. (And it doesn't even generate a messageerror event, the SharedArrayBuffer becomes null somehow.)

I'm not really sure what the best course of action for that scenario is and we're hitting a similar thing in whatwg/html#4734 where this enforcement of boundaries becomes even more critical from a security perspective. Basically, it would be really good if we could have enforcement the moment we cross a boundary, regardless of whether the receiving port is active or not. However, this would basically mean specifying the "agent cluster hop" more explicitly and allowing replacing problematic serialized records with errors there in some form.

(An alternative might be not sending anything across until the receiving port is activated (or at least not from the point where a message contains a problematic object), but then implementations still need to have the above kind of enforcement at the boundary to avoid leaking data in the wrong places. This wouldn't be observable so the specification wouldn't have to do that, but it's always suboptimal if there's such a large architectural gap between implementation and specification.)

@annevk
Copy link
Member Author

annevk commented Oct 18, 2019

Thoughts anyone? It would be nice to make progress here.

@foolip
Copy link
Member

foolip commented Oct 18, 2019

I could review the tests, but probably @yutakahirano is a better reviewer?

@yutakahirano
Copy link
Contributor

I'm confused by cross-origin-resource-policy: cross-site. IIUC it should be cross-origin, not cross-site. WICG/cross-origin-embedder-policy#1

Is my understanding stale?

@annevk
Copy link
Member Author

annevk commented Oct 18, 2019

No, that was my mistake.

@foolip
Copy link
Member

foolip commented Oct 18, 2019

@annevk sent a fix: #19786

@yutakahirano
Copy link
Contributor

(Sorry, I will come back on Oct 23)

@domenic
Copy link
Member

domenic commented Oct 21, 2019

It seems like from the spec's perspective the model might be fine? I.e. the test seems to be testing what the spec is saying, and the spec seems to have a model that's reasonable from a user's perspective. I guess that's what you're getting at in your last paragraph.

If we want to change the spec, and thus the test expectations here, I'd like to hear more from implementations on how exactly they implement MessagePort queues, and in particular how SABs fit in to those. My guess is that it's fairly similar to the spec, except that instead of the serialization containing the [[ArrayBufferData]], [[ArrayBufferLength]], and an agent cluster, it's more likely some kind of pointer/agent cluster ID.

As for implementation enforcement, I don't think there's any danger of implementations leaking SABs across processes, since cross-process shared memory isn't much of a thing. And if two sites/origins are in the same process (e.g. due to resource constraints), then there's no security boundary to be enforced on a technical level; we just want an interop story for when things fail. So I don't quite get the concerns there.

@yutakahirano
Copy link
Contributor

+1 to @domenic, but I'm not familiar with SAB serialization implementation in chromium, too.

@annevk
Copy link
Member Author

annevk commented Oct 23, 2019

I'm happy with continuing to see these as implementation bugs. I think we should push harder for better test coverage with new serializable/transferable objects to account for these various edge cases in order to convince implementers to tackle their infrastructure issues.

@surma was doing some great work with generalizing some of those tests for the new structuredClone() API I remember, but I'm not sure where that ended up and how much it covers the multiple agent cluster cases.

@annevk annevk force-pushed the annevk/messagechannel-sab branch from 4e8f51d to c6c8934 Compare October 23, 2019 13:13
@annevk
Copy link
Member Author

annevk commented Oct 23, 2019

If that's the course of action I guess we should land this test, close whatwg/html#4209, and maybe file a bug against Chrome? (Firefox already has one for MessageChannel/BroadcastChannel not working.)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test code LGTM. I would still like someone to double-check the headers, as I haven't yet figured those out, but maybe @yutakahirano already did that.

@surma
Copy link
Contributor

surma commented Oct 23, 2019

We have the “structured clone battery of tests”, which currently does not contain any tests with SharedArrayBuffers. IIRC, this battery of tests is used by workers, shared workers and I think some more. It should be easy (and great!) if the tests for MessagePorts would also use it.

If someone adds some tests for SABs, this tests would be run against all of the above, which seems desirable.

@annevk
Copy link
Member Author

annevk commented Oct 25, 2019

@surma ah great, pushed some fixes for that to #19886 (and analysis as to why SharedArrayBuffer testing there probably is not worth it).

@yutakahirano final thoughts?

@yutakahirano
Copy link
Contributor

yutakahirano commented Oct 25, 2019

LGTM.

I have no concerns for security.

I don't have any negative opinions for the test expectation, but that can come from my unfamiliarity for MessagePort + SAB implementation in Chromium, so it is possible that another Chromium developer raises concerns from implementer POV in the future.

@annevk annevk merged commit f4f35fb into master Oct 28, 2019
@annevk annevk deleted the annevk/messagechannel-sab branch October 28, 2019 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants