1184626 - Service worker receives duplicate push events for a single push

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Eitan Isaacson [:eeejay]]

I actually see a lot of duplicate push events when i develop a new app. But I managed to at least reproduce the case where two events are dispatched in a mochitest.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: test case

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1184626 - Add a per-process push message listener. r?smaug

Bug 1184626 - Add a per-process push message listener. r?smaug

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c6b0f988f60

Comment 4

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

(I tried MozReview again, but ended up downloading the diff after all. Much easier for me to see the changes that way. Silly old brains too used to the old habits.)

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=076adf490889

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=505853108065

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2603fb3e8f71329a5380b4d313848a7f28d328b2
changeset:  2603fb3e8f71329a5380b4d313848a7f28d328b2
user:       Eitan Isaacson <[e-mail]>
date:       Fri Jul 17 14:54:38 2015 -0700
description:
Bug 1184626 - Add a per-process push message listener. r=smaug

Comment 8

[Robert Bindar]

> Cu.import("resource://gre/modules/PushServiceChildPreload.jsm");

You loaded this module in toolkit/process-content.js, but unfortunately this is not loaded in b2g.
Is there anywhere else we could move this line for the module to actually be loaded on all of our platforms?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2603fb3e8f71

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Robert Bindar from comment #8)
> > Cu.import("resource://gre/modules/PushServiceChildPreload.jsm");
> 
> You loaded this module in toolkit/process-content.js, but unfortunately this
> is not loaded in b2g.
> Is there anywhere else we could move this line for the module to actually be
> loaded on all of our platforms?

We are going to need this for b2g (bug 1151180). Nikhil can you respond to Robert's question?

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Robert Bindar from comment #8)
> > Cu.import("resource://gre/modules/PushServiceChildPreload.jsm");
> 
> You loaded this module in toolkit/process-content.js, but unfortunately this
> is not loaded in b2g.
> Is there anywhere else we could move this line for the module to actually be
> loaded on all of our platforms?

My apologies. When I added the blocking, I did not realize the ni? would get cleared.

If we use system messages to deal with spawning/messaging a child process, then we don't need this. In that case, we'd have to modify PushService#notifyApp() to use the right interfaces on the parent side for system messages.

Comment 12

[Robert Bindar]

Thanks Nikhil!

For now I added that line to dom/ipc/preload.js and with a small fix for Bug 1187481, the push api works ok for both desktop and b2g.

I'll take a look on how to take care of this with system messages, maybe this will also fix the bug when the app is killed and a notification comes in.

Quick question: is there any way we could send the system message to the right child (that susbscribed the for push) and not broadcast to all the children (potentially malicious ones too)?

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Robert Bindar from comment #12)
> Thanks Nikhil!
> 
> For now I added that line to dom/ipc/preload.js and with a small fix for Bug
> 1187481, the push api works ok for both desktop and b2g.
> 
> I'll take a look on how to take care of this with system messages, maybe
> this will also fix the bug when the app is killed and a notification comes
> in.
> 
> Quick question: is there any way we could send the system message to the
> right child (that susbscribed the for push) and not broadcast to all the
> children (potentially malicious ones too)?

This would require modifying the system messages code to attach some sort of unique id along with the 'push' "signal". I'm not sure that is worth the effort until we allow data to be sent in the push.