Closed Bug 1472238 Opened Last year Closed Last year

Some refinements to BroadcastService

Categories

(Core :: DOM: Push Notifications, enhancement, P2)

54 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

References

Details

Attachments

(3 files)

There are some outstanding issues with the client-side support for Megaphone:

- Changing the alwaysConnect pref does not cause the client to reconnect/stop connecting to the connection node. This is necessary to do a pref-flip study.

- PushBroadcastService.addListener doesn't validate that the version ID is sane. In particular, apparently empty strings are not acceptable to the connection nodes.

- When a component registers after having already registered once, there are two sources of truth for the version ID: one implicit in the connection to Megaphone, and one coming from the component call to addListener. We should check that they're the same, log a warning if they aren't, and in either case, we should prefer the one we already have from Megaphone, since that's the one we've already delivered.
I didn't write unit tests for the last one but I think I'm out of time. Hopefully this moves the ball forward a little bit..
Comment on attachment 8988790 [details]
Bug 1472238: hook up alwaysConnect pref changes

https://reviewboard.mozilla.org/r/253970/#review260774

This makes sense, but I wonder if we can simplify.

::: dom/push/PushService.jsm:310
(Diff revision 1)
> +              newState = this.getAllUnexpired().then(records =>
> +                records.length > 0);
> +            }
> +          }
>            this._stateChangeProcessEnqueue(_ =>
> -            this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled"))
> +            newState.then(state => this._changeStateConnectionEnabledEvent(state))

Do we need to handle `alwaysConnect` specially? IIUC, with your change:

* If `!connection.enabled`, we'll disconnect in `_changeStateConnectionEnabledEvent`.
* If `connection.enabled && alwaysConnect`, we'll try to connect in `_changeStateConnectionEnabledEvent`, which calls into `_changeStateOfflineEvent`, which then calls `getAllUnexpired` and checks `alwaysConnect` again.
* If `connection.enabled && !alwaysConnect`, we'll call `getAllUnexpired`, then `_changeStateConnectionEnabledEvent`. If we have records, `_changeStateConnectionEnabledEvent` will call `_changeStateOfflineEvent`, which will call `getAllUnexpired` again.

Would it still work if we kept the body of this block as before? That way:

* If the user flips `connection.enabled` to false, the service will disconnect.
* If we flip `alwaysConnect` to true, the service will check `connection.enabled`. If that's true, `_changeStateOfflineEvent` will connect unconditionally, since it also checks `alwaysConnect`.
* If we flip `alwaysConnect` to false, `_changeStateOfflineEvent` will fall back to checking for records.

::: dom/push/PushService.jsm:514
(Diff revision 1)
>      Services.obs.addObserver(this, "network:offline-status-changed");
>  
>      // Used to monitor if the user wishes to disable Push.
>      prefs.observe("connection.enabled", this);
> +    // Used to load-test the server-side infrastructure for broadcast.
> +    prefs.observe("alwaysConnect", this);

Please add a matching `prefs.ignore` call to https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/push/PushService.jsm#587.
Attachment #8988790 - Flags: review?(kit)
Comment on attachment 8988791 [details]
Bug 1472238: ensure version is not an empty string

https://reviewboard.mozilla.org/r/253972/#review260776

::: dom/push/PushBroadcastService.jsm:117
(Diff revision 3)
>      await this.initializePromise;
>      this._validateSourceInfo(sourceInfo);
>      if (typeof version !== "string") {
>        throw new TypeError("version should be a string");
>      }
> +    if (version === "") {

Nit: `!version` is shorter. :-)
Attachment #8988791 - Flags: review?(kit) → review+
Comment on attachment 8988792 [details]
Bug 1472238: compare old and new versions and prefer what we had

https://reviewboard.mozilla.org/r/253974/#review260784

::: dom/push/PushBroadcastService.jsm:132
(Diff revision 3)
>  
>      // Update listeners before telling the pushService to subscribe,
>      // in case it would disregard the update in the small window
>      // between getting listeners and setting state to RUNNING.
> -    this.jsonFile.data.listeners[broadcastId] = {version, sourceInfo};
> +    //
> +    // Keep the old version (if we have it) because Megaphone is

Hmm. Is there a way for a component to force Megaphone to use the version passed to `addListener`?

If we want to support that in the future, would it make sense to store the `lastAddedVersion` (`lastClientVersion`? `lastResetVersion`? I'm not sure what to call it) in the JSON file? If it's the same as what the component passed to `addListener`, use Megaphone's version; if not, update `lastAddedVersion` and call `subscribeBroadcast`.

If not...can we still avoid calling `saveSoon` and `subscribeBroadcast` if the version doesn't change?
Attachment #8988792 - Flags: review?(kit)
Priority: -- → P2
Comment on attachment 8988790 [details]
Bug 1472238: hook up alwaysConnect pref changes

https://reviewboard.mozilla.org/r/253970/#review266500
Attachment #8988790 - Flags: review?(lina) → review+
ni? for comment 11. :-) And welcome back! 👋
Flags: needinfo?(eglassercamp)
Assignee: nobody → eglassercamp
Comment on attachment 8988792 [details]
Bug 1472238: compare old and new versions and prefer what we had

https://reviewboard.mozilla.org/r/253974/#review260784

> Hmm. Is there a way for a component to force Megaphone to use the version passed to `addListener`?
> 
> If we want to support that in the future, would it make sense to store the `lastAddedVersion` (`lastClientVersion`? `lastResetVersion`? I'm not sure what to call it) in the JSON file? If it's the same as what the component passed to `addListener`, use Megaphone's version; if not, update `lastAddedVersion` and call `subscribeBroadcast`.
> 
> If not...can we still avoid calling `saveSoon` and `subscribeBroadcast` if the version doesn't change?

I didn't really understand the question. Any component can of course tell Megaphone any version whatsoever as its "last known version". But Megaphone is closer to the source of truth, so if the component says anything besides what Megaphone has said, then it's wrong.

Even if the version doesn't change, the `sourceInfo` could have changed (there could have been a refactor and a component moved from one module to another), so we still have to call `saveSoon`. In theory we could optimize this, but I don't think it's worth the extra complexity. However, if the version hasn't changed, then that means this broadcaster isn't new, which means we already don't call `subscribeBroadcast` again.
Sorry, I forgot to click "publish" on a comment!
Flags: needinfo?(eglassercamp)
Comment on attachment 8988790 [details]
Bug 1472238: hook up alwaysConnect pref changes

https://reviewboard.mozilla.org/r/253970/#review260774

> Do we need to handle `alwaysConnect` specially? IIUC, with your change:
> 
> * If `!connection.enabled`, we'll disconnect in `_changeStateConnectionEnabledEvent`.
> * If `connection.enabled && alwaysConnect`, we'll try to connect in `_changeStateConnectionEnabledEvent`, which calls into `_changeStateOfflineEvent`, which then calls `getAllUnexpired` and checks `alwaysConnect` again.
> * If `connection.enabled && !alwaysConnect`, we'll call `getAllUnexpired`, then `_changeStateConnectionEnabledEvent`. If we have records, `_changeStateConnectionEnabledEvent` will call `_changeStateOfflineEvent`, which will call `getAllUnexpired` again.
> 
> Would it still work if we kept the body of this block as before? That way:
> 
> * If the user flips `connection.enabled` to false, the service will disconnect.
> * If we flip `alwaysConnect` to true, the service will check `connection.enabled`. If that's true, `_changeStateOfflineEvent` will connect unconditionally, since it also checks `alwaysConnect`.
> * If we flip `alwaysConnect` to false, `_changeStateOfflineEvent` will fall back to checking for records.

Yes, that works. Sorry, I misread the call graph somehow in my rush to get this out.

> Please add a matching `prefs.ignore` call to https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/push/PushService.jsm#587.

Yes, thanks!
(In reply to Ethan Glasser-Camp (:glasserc) from comment #17)
> I didn't really understand the question. Any component can of course tell
> Megaphone any version whatsoever as its "last known version". But Megaphone
> is closer to the source of truth, so if the component says anything besides
> what Megaphone has said, then it's wrong.

Oh, I see. So once a component registers with Megaphone, then Megaphone manages the version...the component can't change the version later?

> Even if the version doesn't change, the `sourceInfo` could have changed
> (there could have been a refactor and a component moved from one module to
> another), so we still have to call `saveSoon`. In theory we could optimize
> this, but I don't think it's worth the extra complexity.

Sounds good, I agree about avoiding the extra complexity. Thanks!
The component gets to say "The last version I know about is V1", and we report this to Megaphone... but if Megaphone tells us "The current up-to-date version is V2", there's no way for us to convince it otherwise. This fix is about what happens on the next restart, when we connect to Megaphone with some extant version, and then eventually the component tries to reregister (because why not, it's idempotent), and the version number it gives us is still V1 (or something else entirely). If this happens, there must be a bug with either the message handler or whatever code is calculating the version ID, but it's not anything we can do anything about, and Megaphone is the source of truth anyhow.
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
(In reply to Ethan Glasser-Camp (:glasserc) from comment #21)
> The component gets to say "The last version I know about is V1", and we
> report this to Megaphone... but if Megaphone tells us "The current
> up-to-date version is V2", there's no way for us to convince it otherwise.

Got it. Thanks for the excellent explanation!
Comment on attachment 8988792 [details]
Bug 1472238: compare old and new versions and prefer what we had

https://reviewboard.mozilla.org/r/253974/#review266512
Attachment #8988792 - Flags: review?(lina) → review+
Keywords: checkin-needed
There is still one open issue: "Bug 1472238: ensure version is not an empty string", please try to fix this in order to land this bug.
Keywords: checkin-needed
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcb897d35e71
hook up alwaysConnect pref changes r=lina
https://hg.mozilla.org/integration/autoland/rev/fcc52b742b2f
ensure version is not an empty string r=lina
https://hg.mozilla.org/integration/autoland/rev/dea043963353
compare old and new versions and prefer what we had r=lina
You need to log in before you can comment on or make changes to this bug.