1259164 - ServiceWorkerMessageEvent.origin is not set correctly

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Edwin Wong [:edwong]]

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.

Comment 1

[Edwin Wong [:edwong]]

: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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Still need a reduced test case, but my guess is we're not using switchToTabHavingURI within the service worker code.

Comment 3

[Edwin Wong [:edwong]]

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

Comment 4

[:Harald Kirschner :digitarald]

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

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 1218148 is about implementing WindowClient.navigate().

Comment 6

[Andrew Overholt [:overholt]]

(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 ...

Comment 7

[(no longer active)]

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

[n8s]

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

[n8s]

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.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 11

[(no longer active)]

Yeah looks like we incorrectly set mOrigin to an empty string here: <https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerClient.cpp#140>

Comment 12

[(no longer active)]

Attachment: Set ServiceWorkerMessageEvent.origin correctly when calling ServiceWorkerClient.postMessage()

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

[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.

Comment 17

[(no longer active)]

(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

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 19

[Pulsebot]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/110003172c5b

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Ben or Ehsan, do you think this would be good to uplift or is that not necessary/too risky?

Comment 22

[(no longer active)]

(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!

Comment 23

[n8s]

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 :)

Comment 24

[(no longer active)]

(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!

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(Fixing tracking flags that were intended to be reset by comment #24)

Comment 26

[(no longer active)]

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.  :-)

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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!