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

worker.onmessage is seeing non-empty event.origin #25149

Closed
pshaughn opened this issue Dec 6, 2019 · 9 comments
Closed

worker.onmessage is seeing non-empty event.origin #25149

pshaughn opened this issue Dec 6, 2019 · 9 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 6, 2019

This happens in WPT workers/interfaces/DedicatedWorkerGlobalScope/postMessage/message-event.html. I don't see anything explicit in the standard saying to set origin to an empty string here, but I'm also not seeing anything in the worker or message port steps saying to set it to a real origin, so the implication must be to leave it default, and browsers are passing this test.

@jdm jdm added the A-content/dom label Dec 6, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 6, 2019
@gterzian gterzian mentioned this issue Dec 8, 2019
0 of 7 tasks complete
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 2, 2020

Hi! I want to try and solve this issue, if it's not a problem.
I see that the function to be edited is this:

pub fn new_initialized(
global: &GlobalScope,
data: HandleValue,
origin: DOMString,
source: Option<&WindowProxyOrMessagePortOrServiceWorker>,
lastEventId: DOMString,
ports: Vec<DomRoot<MessagePort>>,
) -> DomRoot<MessageEvent> {

Should I completely remove the origin parameter it is accepting here (and make the required changes in all of its calls) or should it be left unutilized?

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 2, 2020

MessageEvent is used in many different ways; some of them are specified as explicitly setting origin, and some of them aren't. If you click on the bolded word "origin" in https://html.spec.whatwg.org/multipage/comms.html#dom-messageevent-origin you should get a popup of all the places the HTML specification references it.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 2, 2020

Oh, my mistake. I was not aware of all the different specs that it supported. It will take me some time, but I'll try to understand the usage (especially the posting messages section) and the serialization of origin, and will update my status soon.

@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2020

let messageevent = MessageEvent::new(
scope,
atom!("message"),
false,
false,
message,
DOMString::from(origin.unwrap_or("")),
source
.map(|source| {
WindowProxyOrMessagePortOrServiceWorker::WindowProxy(DomRoot::from_ref(source))
})
.as_ref(),
DOMString::new(),
ports,
);
is the code that creates the MessageEvent that is used for worker.onmessage. Looks like we're explicitly passing a non-empty origin value there; we should always pass an empty string instead.

@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2020

That was the wrong code link - I meant to link

if let Ok(ports) = structuredclone::read(&global, data, message.handle_mut()) {
MessageEvent::dispatch_jsval(
target,
&global,
message.handle(),
Some(&origin),
None,
ports,
);
, where we should pass a None value instead of the origin.

@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2020

In fact, we should remove the origin argument from Worker::handle_message entirely.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 3, 2020

Thanks for pointing that out. That will save a lot of time :)

Just out of curiosity, while I was looking around, I found this:

pub fn ascii_serialization(&self) -> String {
self.clone().into_url_origin().ascii_serialization()
}
}

I cannot understand how this works. The recursive call (I could not find any other declaration of ascii_serialization, so I figured it must be recursive) is confusing me a bit. Can you explain this briefly?

@jdm
Copy link
Member

@jdm jdm commented Jan 3, 2020

into_url_origin returns a url::Origin, which comes from the rust-url crate (which is different than components/url).

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 3, 2020

Oh, I see. Now I get it. Thanks:)

bors-servo added a commit that referenced this issue Jan 3, 2020
Remove `origin` parameter from `Worker::handle_message`

<!-- Please describe your changes on the following line: -->
Test in  `workers/interfaces/DedicatedWorkerGlobalScope/postMessage/message-event.html` was failing because worker.onmessage was seeing non-empty event.origin. Removing `origin` parameter from `Worker::handle_message` and sending `None` to `MessageEvent::dispatch_jsval` solves the issue.

r?@jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25149  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 4, 2020
Remove `origin` parameter from `Worker::handle_message`

<!-- Please describe your changes on the following line: -->
Test in  `workers/interfaces/DedicatedWorkerGlobalScope/postMessage/message-event.html` was failing because worker.onmessage was seeing non-empty event.origin. Removing `origin` parameter from `Worker::handle_message` and sending `None` to `MessageEvent::dispatch_jsval` solves the issue.

r?@jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25149  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo bors-servo closed this in afa1b85 Jan 4, 2020
web-platform-test failures automation moved this from To do to Done Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.