ServiceWorkerMessageEvent.origin is not set correctly

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: edwong, Assigned: Away for a while)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46-, firefox47-, firefox48+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
We have a partner reporting that code in chrome opens a URL in a open tab from it's same domain while this code opens a new tab in Firefox.  It's not clear of  root cause.
(Reporter)

Updated

2 years ago
Blocks: 1259207
(Reporter)

Comment 1

2 years ago
:jaws from thread said: Notifications have been refocusing their originating tab since https://bugzilla.mozilla.org/show_bug.cgi?id=853972 landed, so this must be ServiceWorker related. Sites can opt-out of this behavior by using event.preventDefault(). Can you please file a bug for the ServiceWorker implementation? We will need a reduced testcase, and I think Kit or William Chen will be more familiar with the code there.
Still need a reduced test case, but my guess is we're not using switchToTabHavingURI within the service worker code.
(Reporter)

Comment 3

2 years ago
I'm wondering if they're using client.navigate().  Anyhow, the best course of action maybe connecting someone directing with the partner dev, then we won't be guessing.
https://www.w3.org/TR/service-workers/#client-navigate

http://stackoverflow.com/questions/32504823/serviceworker-chrome-push-notifications-forward-to-new-page
Most reduced test case (acticates service worker and navigates on client). Works in Chrome and doesn't work in Firefox: https://googlechrome.github.io/samples/service-worker/windowclient-navigate/index.html
Flags: needinfo?(jaws)
Bug 1218148 is about implementing WindowClient.navigate().
(In reply to Edwin Wong [:edwong] from comment #3)
> I'm wondering if they're using client.navigate().

Can we confirm this? As Panos noted, we don't support this yet.

There were thoughts yesterday about perhaps https://developer.mozilla.org/en-US/docs/Web/API/Clients/matchAll not finding the open existing tab ...
(Assignee)

Comment 7

2 years ago
Note that they can use a debugger to figure out what exactly is going wrong: <https://hacks.mozilla.org/2016/03/debugging-service-workers-and-push-with-firefox-devtools/>.  Also it would be much better if we can get an engineer on their side familiar with the issue to comment on the bug.  :-)

Comment 8

2 years ago
So I think that this is an issue with client.postMessage. We aren't actually using WindowClient.navigate() instead we post a message to the page asking it to navigate, this way if the user is currently composing a post or message on the page we won't just blow it away. The client then needs to ack the navigation for us to then focus on it. (I go into a bit more detail in a talk I recently gave, https://youtu.be/8XLHdetDIgw?t=18m34s)

From what I can tell, clients are currently not getting messages sent to client.postMessage, also the post message demo at https://googlechrome.github.io/samples/service-worker/post-message/index.html does not seem to be working, so I'm wondering if something around post message is currently broken.

Comment 9

2 years ago
So it looks like the issue here is due to the fact that in Firefox when listening to messages via "navigator.serviceWorker.addEventListener('message'..." the value returned from event.origin is an empty string, but in Chrome the value is the scope of the service worker. We're working on fixing our implementation to correctly deal with this. However, this is one area of difference between Chrome and Firefox.
(In reply to n8s from comment #9)
> So it looks like the issue here is due to the fact that in Firefox when
> listening to messages via
> "navigator.serviceWorker.addEventListener('message'..." the value returned
> from event.origin is an empty string, but in Chrome the value is the scope
> of the service worker. We're working on fixing our implementation to
> correctly deal with this. However, this is one area of difference between
> Chrome and Firefox.

Nikhil, does this sound correct to you?
Flags: needinfo?(jaws) → needinfo?(nsm.nikhil)
(Assignee)

Comment 11

2 years ago
Yeah looks like we incorrectly set mOrigin to an empty string here: <https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerClient.cpp#140>
Flags: needinfo?(nsm.nikhil)
Summary: Investigate issue of Service worker not being able to open a URL in an existing tab → ServiceWorkerMesssageEvent.origin is not set correctly
(Assignee)

Updated

2 years ago
Blocks: 1143717
(Assignee)

Comment 12

2 years ago
Created attachment 8735445 [details] [diff] [review]
Set ServiceWorkerMessageEvent.origin correctly when calling ServiceWorkerClient.postMessage()
Attachment #8735445 - Flags: review?(bkelly)
(Assignee)

Updated

2 years ago
Assignee: nobody → ehsan
Blocks: 1226983
Component: DOM: Push Notifications → DOM: Service Workers
Summary: ServiceWorkerMesssageEvent.origin is not set correctly → ServiceWorkerMessageEvent.origin is not set correctly

Comment 13

2 years ago
The spec text seems confusing to me:

  "The origin attribute must return the value it was initialized to. When the object is created, this attribute must be initialized to the empty string. It represents the origin of the service worker’s environment settings object from which the message is sent."

These sentences seem to contradict each other...

Comment 14

2 years ago
I wrote a spec issue.  I think we need to understand if this should really be an empty string for some reason or not before landing this:

  https://github.com/slightlyoff/ServiceWorker/issues/859

Comment 15

2 years ago
Comment on attachment 8735445 [details] [diff] [review]
Set ServiceWorkerMessageEvent.origin correctly when calling ServiceWorkerClient.postMessage()

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

r=me with comments addressed.  Thanks!

Also, can you add a check to one of our tests?

::: dom/workers/ServiceWorkerClient.cpp
@@ +135,5 @@
>      }
>  
>      RootedDictionary<ServiceWorkerMessageEventInit> init(aCx);
>  
> +    nsCOMPtr<nsIPrincipal> principal = aTargetContainer->GetParentObject()->PrincipalOrNull();

I think GetParentObject() can return nullptr.  Its ultimately returned from a weak reference.  I know this code assumes non-nullptr elsewhere, but it seems at least an assert here would be good.

@@ +142,2 @@
>      init.mData = messageData;
> +    if (principal) {

I think we should check for at least null principal and system principal here.  I doubt we want to expose our origin "moz-nullprincipal" serialization to script.

Maybe we can just assert this?  Or we could return empty string for null principal.

Not sure if we should check for system principal here.

@@ +144,5 @@
> +      nsAutoCString origin;
> +      principal->GetOrigin(origin);
> +      init.mOrigin.Construct(NS_ConvertUTF8toUTF16(origin));
> +    } else {
> +      init.mOrigin.Construct(EmptyString());

Can you just move the nsAutoCString and init.mOrigin.Construct() out of the conditional statement?  That way if we don't trigger the if-statement then origin string is default empty string and you can just call Construct() with it either way.  Just simplifies the logic a bit here.
Attachment #8735445 - Flags: review?(bkelly) → review+
[Tracking Requested - why for this release]: Requesting tracking for Fx46 and Fx47 since this is an issue that a partner has brought up and affects webcompat between Chrome and Firefox.
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
(Assignee)

Comment 17

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #15)
> ::: dom/workers/ServiceWorkerClient.cpp
> @@ +135,5 @@
> >      }
> >  
> >      RootedDictionary<ServiceWorkerMessageEventInit> init(aCx);
> >  
> > +    nsCOMPtr<nsIPrincipal> principal = aTargetContainer->GetParentObject()->PrincipalOrNull();
> 
> I think GetParentObject() can return nullptr.  Its ultimately returned from
> a weak reference.  I know this code assumes non-nullptr elsewhere, but it
> seems at least an assert here would be good.

It can only return null after the window object has been destroyed AFAICT, which cannot happen while we're dispatching a DOM event...  But I'll add an assert anyway.

> @@ +142,2 @@
> >      init.mData = messageData;
> > +    if (principal) {
> 
> I think we should check for at least null principal and system principal
> here.  I doubt we want to expose our origin "moz-nullprincipal"
> serialization to script.
> 
> Maybe we can just assert this?  Or we could return empty string for null
> principal.
> 
> Not sure if we should check for system principal here.

I'll add the checks, but I'm not sure what you're worrying about here.  I cannot think of a possible scenario where we would either deal with a null or a system principal here, since a page running under either principal should never have a Client constructed for it...  If you can think of a case where that won't be true, please file follow-up bugs, since those seem like disastrous scenarios we should fix regardless of what origin string we decide to pass here.  :-)

> @@ +144,5 @@
> > +      nsAutoCString origin;
> > +      principal->GetOrigin(origin);
> > +      init.mOrigin.Construct(NS_ConvertUTF8toUTF16(origin));
> > +    } else {
> > +      init.mOrigin.Construct(EmptyString());
> 
> Can you just move the nsAutoCString and init.mOrigin.Construct() out of the
> conditional statement?  That way if we don't trigger the if-statement then
> origin string is default empty string and you can just call Construct() with
> it either way.  Just simplifies the logic a bit here.

Sure.

Comment 18

2 years ago
An assert that its not a null or system principal seems fine to me.

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/110003172c5b

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/110003172c5b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Ben or Ehsan, do you think this would be good to uplift or is that not necessary/too risky?
Flags: needinfo?(ehsan)
Flags: needinfo?(bkelly)
(Assignee)

Comment 22

2 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Ben or Ehsan, do you think this would be good to uplift or is that not
> necessary/too risky?

This patch isn't really risky, but I would like confirmation from FB first to see whether it actually fixes all of the issues they've been seeing, and also whether this is something that they will work around on their side or whether they prefer us to backport.

n8s, this will be in tomorrow's Nightly which you can download from http://nightly.mozilla.org/.  Can you please test to see whether tomorrow's Nightly build fixes the issue you've been seeing on your side?  Thanks a lot!
Flags: needinfo?(n8s)
Flags: needinfo?(ehsan)
Flags: needinfo?(bkelly)

Comment 23

2 years ago
We've already implemented a workaround (which will be live shortly) so if this doesn't get backported then that's totally fine with us :)
Flags: needinfo?(n8s)
(Assignee)

Comment 24

2 years ago
(In reply to n8s from comment #23)
> We've already implemented a workaround (which will be live shortly) so if
> this doesn't get backported then that's totally fine with us :)

OK, thanks!  All other things being equal, I prefer to not backport changes to minimize risk on Aurora and Beta, so since you've worked around this, let's have this change ride the trains.  Resetting the tracking flags based on this.

Thanks for helping us find and fix this issue!
tracking-firefox48: --- → ?
(Fixing tracking flags that were intended to be reset by comment #24)
tracking-firefox46: ? → ---
tracking-firefox47: ? → ---
tracking-firefox48: ? → ---
(Assignee)

Comment 26

2 years ago
I'm still interested in keeping this bug on the release management's radar to give them a chance to evaluate and potentially disagree with comment 24.  :-)
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
It sounds like there is a FB workaround already, so I think it's fine to let this ride with 48. 
I'll track it for 48 so that we'll notice if this bug reopens. Thanks Ehsan!
tracking-firefox46: ? → -
tracking-firefox47: ? → -
tracking-firefox48: ? → +
You need to log in before you can comment on or make changes to this bug.