Support "global broadcast"/real time updates API

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: glasserc, Assigned: glasserc)

Tracking

54 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(5 attachments)

The "global broadcast" a.k.a. Megaphone a.k.a. "real time updates" project is meant to allow us to push certain kinds of update to the browser immediately, without waiting for code in the browser to "phone home" to check if anything has changed. See https://docs.google.com/document/d/17zPQ_uARAsK41_cB_aTlzUzWXzNXefF8cv-oToVa3QY for more information.

This bug represents the work to develop this feature in Firefox ("Create a prototype client side library in Firefox"). The overall game plan is to start with something in the dom/push directory which allows code to register interest in/be notified about broadcast events, and then work with :benbangert and the browser architecture group to figure out where this code should live long-term.
Priority: -- → P2
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review241470

::: dom/push/PushService.jsm:492
(Diff revision 1)
>          this._changeServerURL(options.serverURI, STARTING_SERVICE_EVENT, options));
>  
>      } else {
>        // This is only used for testing. Different tests require connecting to
>        // slightly different URLs.
> -      this._stateChangeProcessEnqueue(_ =>
> +      return this._stateChangeProcessEnqueue(_ =>

I made these changes to make it easier to test, specifically to make it easier to know when the service was fully operational. This wasn't the only way to do it, but I didn't see a reason not to do this.

::: dom/push/PushServiceAndroidGCM.jsm:199
(Diff revision 1)
>      });
>    },
>  
> +  sendSubscribeBroadcast: async function(serviceId, version) {
> +    // Not implemented yet
> +  },

I wasn't sure what to do with the GCM and Http2 connectors.
Flags: needinfo?(bugzilla)
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review241516

::: dom/push/BroadcastService.jsm:122
(Diff revision 1)
> +    this.jsonFile = jsonFile;
> +    this.loadPromise = this.jsonFile.load();
> +  }
> +
> +  /**
> +   * Return an array of {serviceId, version, componentInfo}

Do you need a whitelist or some other mechanism here for acceptable service IDs? Can anything bad happen if an external entity is able to write into this file?
Flags: needinfo?(bugzilla)
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review241516

> Do you need a whitelist or some other mechanism here for acceptable service IDs? Can anything bad happen if an external entity is able to write into this file?

Oh, I didn't think of that. I assume that Cu.import only allows access to local code, and so that isn't a problem because it just devolves into replacing built-in JS in Firefox?
(In reply to Ethan Glasser-Camp (:glasserc) from comment #6)

> Oh, I didn't think of that. I assume that Cu.import only allows access to
> local code, and so that isn't a problem because it just devolves into
> replacing built-in JS in Firefox?

I think the question is: can the presence of text in this file cause code to be executed, or code to be executed unexpectedly?

My first guess is 'no' — that text must be present in this file _and a message received from the server_ for code to be executed, but perhaps an attacker could map a message to the wrong component, causing component initialization and code to run, perhaps causing a crash.
Can you elaborate a little bit on the threat model here? It sounds like you're concerned that an attacker is going to write to the broadcast-listeners.json and thereby crash the browser. But if an attacker has write access to the profile, wouldn't it be  better to install an extension that logs passwords or something?

If we're really concerned about the attacker writing to the broadcast-listeners.json file, then we'd have to use a whitelist. Of course, if we use a whitelist, then there's no real point in the BroadcastStorage component; we'd already have a list of all the broadcast handlers in the codebase. The BroadcastStorage component was designed so that we wouldn't *need* such a list, based on your recommendations, but I guess if you really prefer that we have one, I don't have a strong objection.
Comment on attachment 8967016 [details]
Bug 1440022: Some plausible test cases

https://reviewboard.mozilla.org/r/235688/#review243736

This all makes sense to me, thanks for taking this on, Ethan!

::: dom/push/test/xpcshell/broadcast_handler.jsm:5
(Diff revision 1)
> +"use strict";
> +
> +var EXPORTED_SYMBOLS = ["broadcastHandler"];
> +
> +var getBroadcastHandler = function() {

I think this should be in `EXPORTED_SYMBOLS`, too; see my note about `Cu.import` in part 3.

::: dom/push/test/xpcshell/moz.build:2
(Diff revision 1)
>  EXTRA_COMPONENTS += [
> +    'broadcast_handler.jsm',

Nit: I think this technically belongs in `EXTRA_JS_MODULES`, not `EXTRA_COMPONENTS`, but I'm not sure if the distinction matters in practice.

::: dom/push/test/xpcshell/test_broadcast_storage.js:35
(Diff revision 1)
> +    saveSoon: function() {
> +      this.saved = true;
> +    },
> +  };
> +
> +  let broadcastStorage = new BroadcastStorage(jsonFileMock);

Instead of mocking out `JSONFile` completely, another idea would be to pass a path to a temporary file, similar to form autofill: https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/browser/extensions/formautofill/test/unit/head.js#63,67-70 The API is so simple that either approach would work, though. I tend to avoid mocks unless they're absolutely necessary (we have several complex ones in Sync, and sometimes end up testing against incorrect ones), but that's a personal preference.

::: dom/push/test/xpcshell/test_broadcast_storage.js:49
(Diff revision 1)
> +      }
> +    }
> +  });
> +
> +  await broadcastStorage.upsertListener("broadcast-test-42", "2018-02-02", {
> +    moduleName: "resource://test/broadcast_handler.jsm",

Nit: Let's spell this `moduleURI` or `resourceURI`; these aren't names.

::: dom/push/test/xpcshell/test_broadcast_storage.js:50
(Diff revision 1)
> +    }
> +  });
> +
> +  await broadcastStorage.upsertListener("broadcast-test-42", "2018-02-02", {
> +    moduleName: "resource://test/broadcast_handler.jsm",
> +    attributeName: "getBroadcastHandler",

IIUC, `attributeName` points to a function that returns the handler. Could we simplify and have it name the `handler` directly? I imagine they're going to be singletons in most cases (maybe that's not correct, though?), and we could remove a level of indirection if we have JSMs just export their handlers.

::: dom/push/test/xpcshell/test_broadcast_storage.js:57
(Diff revision 1)
> +  ok(jsonFileMock.saved);
> +  deepEqual(jsonFileMock.data, {
> +    listeners: {
> +      "broadcast-test": {
> +        version: "2018-02-01",
> +        componentInfo: {

Nit: Let's call this `moduleInfo`, since these aren't (XPCOM) components...

::: dom/push/test/xpcshell/test_broadcast_success.js:43
(Diff revision 1)
> +
> +  await PushService.init({
> +    serverURI: "wss://push.example.org/",
> +    db,
> +    makeWebSocket(uri) {
> +      return new MockWebSocket(uri, {

I'm really sorry about how clunky this is... :-(
Attachment #8967016 - Flags: review?(kit)
Comment on attachment 8967017 [details]
Bug 1440022: Some infrastructure for testing broadcast_subscribe

https://reviewboard.mozilla.org/r/235690/#review243776
Attachment #8967017 - Flags: review?(kit) → review+
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review243760

This looks really good, thanks!

::: dom/push/BroadcastService.jsm:1
(Diff revision 1)
> +/* jshint moz: true, esnext: true */

Nit: Let's name this file `PushBroadcastService`, for consistency with the other JSMs, and so that it's not confused with the `BroadcastChannel` DOM API.

::: dom/push/BroadcastService.jsm:12
(Diff revision 1)
> +
> +ChromeUtils.import("resource://gre/modules/osfile.jsm");
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", "resource://gre/modules/JSONFile.jsm");
> +
> +var EXPORTED_SYMBOLS = ["BroadcastService", "BroadcastStorage", "broadcastService"];

How about exporting the `broadcastService` singleton (naming it `pushBroadcastService` to match the module name?), and keeping the implementations unexported?

```js
// You can still get at the unexported symbols via the module's global object in tests...
const {BroadcastStorage, BroadcastService} = ChromeUtils.import(...);

// But consumers will only `ChromeUtils.import` the `pushBroadcastService` singleton.
ChromeUtils.import("resource://gre/modules/PushBroadcastService.jsm");
```

::: dom/push/BroadcastService.jsm:53
(Diff revision 1)
> +   * Reset to a state akin to what you would get in a new profile.
> +   * In particular, wipe anything from storage.
> +   *
> +   * Used mainly for testing.
> +   */
> +  async resetListeners() {

Let's name this `_resetListeners`, or maybe have tests access it via `storage.resetListeners` directly, to discourage consumers from treating this as a public API.

::: dom/push/BroadcastService.jsm:67
(Diff revision 1)
> +    }
> +    this.listeners[serviceId] = {version, componentInfo};
> +    await this.storage.upsertListener(serviceId, version, componentInfo);
> +  }
> +
> +  async receivedBroadcastMessage(data) {

Nit: Let's call this something like `handleBroadcastMessage`, to avoid confusion with the handler's `receivedBroadcastMessage` and for grep-ability.

::: dom/push/BroadcastService.jsm:81
(Diff revision 1)
> +        continue;
> +      }
> +
> +      const {componentInfo} = this.listeners[serviceId];
> +      const {moduleName, attributeName} = componentInfo;
> +      const module = Cu.import(moduleName);

It's best to use `ChromeUtils.import(moduleName, module)` here, I think. `Cu.import` (and `ChromeUtils.import`) without a second argument will dump the imported module's `EXPORTED_SYMBOLS` into the global scope as a side effect. The two-argument form attaches `EXPORTED_SYMBOLS` onto `module`.

::: dom/push/BroadcastService.jsm:113
(Diff revision 1)
> +/**
> + * A separate layer for storing and restoring the current set of
> + * listeners.
> + *
> + * This helps us mock parts of the BroadcastService and swap out the
> + * storage mechanism if something better than JSONFile comes along.

I think this could be flattened into `BroadcastService`, using `JSONFile` directly. The storage requirements and broadcast service interface are nice and simple, and I'm not sure it's worth adding a separate layer for storage.

::: dom/push/BroadcastService.jsm:122
(Diff revision 1)
> +    this.jsonFile = jsonFile;
> +    this.loadPromise = this.jsonFile.load();
> +  }
> +
> +  /**
> +   * Return an array of {serviceId, version, componentInfo}

I may be misunderstanding Richard's concern, but I don't think you need an explicit allow list here.

Firefox modules need to opt in to broadcasting, and the server only knows the service ID and version. The mapping of ID to module is stored on the client only, so a compromised push server can't load arbitrary JSMs. This API is also not exposed to WebExtensions, and WEs don't allow for arbitrary filesystem access.

A rogue Firefox module, or maybe rogue third-party code, could modify the JSON file and also Firefox JSMs...but, at that point, it presumably already has (unrestricted!) access to your entire local filesystem.

::: dom/push/BroadcastService.jsm:157
(Diff revision 1)
> +}
> +
> +function initializeBroadcastService() {
> +  // Fallback path for xpcshell tests.
> +  let path = "broadcast-listeners.json";
> +  if (OS.Constants.Path.profileDir) {

Is this ever falsy? IIRC, xpcshell tests set up a temp profile (you might need to call `do_get_profile` first?)...

::: dom/push/PushService.jsm:243
(Diff revision 1)
>        // Disconnect first.
>        this._service.disconnect();
>      }
>  
>      let records = await this.getAllUnexpired();
> +    let broadcastListeners = await broadcastService.getListeners();

Hmm, don't you need to import your new JSM here?

::: dom/push/PushService.jsm:492
(Diff revision 1)
>          this._changeServerURL(options.serverURI, STARTING_SERVICE_EVENT, options));
>  
>      } else {
>        // This is only used for testing. Different tests require connecting to
>        // slightly different URLs.
> -      this._stateChangeProcessEnqueue(_ =>
> +      return this._stateChangeProcessEnqueue(_ =>

LGTM!

::: dom/push/PushServiceAndroidGCM.jsm:199
(Diff revision 1)
>      });
>    },
>  
> +  sendSubscribeBroadcast: async function(serviceId, version) {
> +    // Not implemented yet
> +  },

I think it's fine to leave them for now. We're going to need to figure out what to do on Android. Do we want to open a WebSocket to Autopush when Android is foregrounded, or deliver broadcast messages through GCM? (I'm also not sure how this all will work in the GeckoView world).

You can definitely ignore H2, I don't think we have any plans to use it for our server.
Attachment #8967018 - Flags: review?(kit)
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #11)
> A rogue Firefox module, or maybe rogue third-party code, could modify the
> JSON file and also Firefox JSMs...but, at that point, it presumably already
> has (unrestricted!) access to your entire local filesystem.

I read Richard's comment 7, and I think we're OK...but it would be good to get Ulfr involved to double-check.

A compromised push server could send a malformed version, which might cause the broadcast handler to do the wrong thing if it passes that version elsewhere...so we *may* want to have handlers, or the broadcast service itself, validate versions. (Though there is an argument for keeping them opaque).

A compromised server could also send a large number of broadcast messages for a service ID, which might increase I/O load or DOS our servers if the handler turns around and makes an HTTP request to our services.
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review243760

> Nit: Let's call this something like `handleBroadcastMessage`, to avoid confusion with the handler's `receivedBroadcastMessage` and for grep-ability.

I named it `receivedBroadcastMessage` for consistency with PushService's `receivedPushMessage`. Or do you think it should have one name in `PushService` and call out to a different name here?

> Is this ever falsy? IIRC, xpcshell tests set up a temp profile (you might need to call `do_get_profile` first?)...

Because `PushService` now imports this module, which defines a singleton `broadcastService`, this code can get run before any `do_get_profile` is run. This is kind of a hack, I'm open to suggestions here..
Comment on attachment 8967016 [details]
Bug 1440022: Some plausible test cases

https://reviewboard.mozilla.org/r/235688/#review243736

> IIUC, `attributeName` points to a function that returns the handler. Could we simplify and have it name the `handler` directly? I imagine they're going to be singletons in most cases (maybe that's not correct, though?), and we could remove a level of indirection if we have JSMs just export their handlers.

The idea was that we wouldn't want to initialize modules ahead of time, and some modules require asynchronous initialization (something more than just executing the code). So the idea here is that we could point to an asynchronous function that could initialize and it would return the handler. On the other hand, I guess we could just as easily make the handler responsible for ensuring that the module was initialized. OK, I'm sold.
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review243760

> Hmm, don't you need to import your new JSM here?

OMG, yes! I guess this worked because of a quirk in how I was importing stuff..
I've pushed a new version of the commit series that addresses the feedback, but in addressing it I noticed a bunch of other things I ought to do:

- I need test to verify that things are OK even if they're uninitialized (empty JSONFile). (Right now all the tests provide their own initial data.)
- Test that version info is updated after delivering a broadcast notification.
- Test that malformed sourceInfo is rejected. This would have helped debugging one of the refactors I did as part of this.

Feel free to review again, or wait for the above, or whatever.
OK, I've addressed those three other outstanding issues! Next is to try to hook up this machinery to the OneCRL machinery.
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review248336


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/blocklist-clients.js:186
(Diff revision 1)
> +  allBlocklistsRemoteSettings.forEach(async client => {
> +    const broadcastID = `remote-settings/${client.bucketName}_${client.collectionName}`;
> +    blocklistClientsByBroadcastID.set(broadcastID, client);
> +
> +    const currentVersion = await client.openCollection(async collection => {
> +      return (await collection.db.getLastModified());

Error: Redundant use of `await` on a return value. [eslint: no-return-await]
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review248360

Nice, I like this API!

::: services/common/blocklist-clients.js:189
(Diff revision 1)
> +
> +    const currentVersion = await client.openCollection(async collection => {
> +      return (await collection.db.getLastModified());
> +    });
> +    const moduleInfo = {
> +      moduleURI: "resource://services-common/blocklist-clients.js",

Protip (I just learned about this today): You can use `__URI__` here, which gives you the current module URI as a string.

https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/js/xpconnect/loader/mozJSComponentLoader.cpp#731-737 wires it up.

::: services/common/blocklist-clients.js:199
(Diff revision 1)
> +  });
>  }
>  
> +var blocklistBroadcastHandler = {
> +  async receivedBroadcastMessage(data, broadcastID) {
> +    initialize();

Is `initialize` safe to call multiple times, if a non-broadcast consumer calls it first?
Attachment #8974122 - Flags: review?(kit) → review+
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review248364

Looking good! The biggest questions I have are around multiple listeners, removing listeners ("we'll never want to do this" is a totally OK answer!), version order, and UAID resets.

::: dom/push/PushBroadcastService.jsm:27
(Diff revision 4)
> +    prefix: "BroadcastService",
> +  });
> +});
> +ChromeUtils.defineModuleGetter(this, "PushService", "resource://gre/modules/PushService.jsm");
> +
> +class InvalidSourceInfo extends Error {

Nit: We might also want to set `name` manually, as in https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/services/sync/modules/util.js#41-42. I think it'll default to `Error` otherwise.

::: dom/push/PushBroadcastService.jsm:39
(Diff revision 4)
> +    this.initializePromise = this.loadListenersFromStorage();
> +  }
> +
> +  /**
> +   * Convert the listeners from our on-disk format to the format
> +   * needed by a subscribe message.

Needed by a hello message?

::: dom/push/PushBroadcastService.jsm:51
(Diff revision 4)
> +    }, {});
> +  }
> +
> +  async loadListenersFromStorage() {
> +    await this.jsonFile.load();
> +    if (!this.jsonFile.data.hasOwnProperty("listeners")) {

You can also use the `dataPostProcessor` option to `JSONFile` to do this, if you want, as in https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/browser/extensions/formautofill/FormAutofillStorage.jsm#1784,1791.

We might also want to add a `version` field to the data, in case we change the storage schema later. I'm not sure what we should do if we get a version newer than what we know about (in the case of a profile downgrade, for example)...probably downgrade the version, but leave the source infos in place, so that we rerun migration code on the next upgrade.

This is all hypothetical, because we don't know how we'll evolve the schema yet, if at all. But it's good to have something in place.

::: dom/push/PushBroadcastService.jsm:73
(Diff revision 4)
> +  /**
> +   * Ensure that a sourceInfo is correct (has the expected fields).
> +   */
> +  _validateSourceInfo(sourceInfo) {
> +    const {moduleURI, attributeName} = sourceInfo;
> +    if (!moduleURI) {

Let's validate that both are strings, too.

::: dom/push/PushBroadcastService.jsm:76
(Diff revision 4)
> +  _validateSourceInfo(sourceInfo) {
> +    const {moduleURI, attributeName} = sourceInfo;
> +    if (!moduleURI) {
> +      throw new InvalidSourceInfo("missing moduleURI");
> +    }
> +    if (!attributeName) {

Nit: What do you think of `symbol` or `symbolName`, instead of `attributeName`? (Just because `XPCOMUtils.jsm` calls this a "symbol", and to match `EXPORTED_SYMBOLS`).

::: dom/push/PushBroadcastService.jsm:85
(Diff revision 4)
> +
> +  /**
> +   * Add an entry for a given listener if it isn't present, or update
> +   * one if it is already present.
> +   */
> +  async addListener(broadcastId, version, sourceInfo) {

Please document these params in the JSDoc comment, too.

::: dom/push/PushBroadcastService.jsm:96
(Diff revision 4)
> +    const isNew = !this.jsonFile.data.listeners.hasOwnProperty(broadcastId);
> +    if (isNew) {
> +      await this.pushService.subscribeBroadcast(broadcastId, version);
> +    }
> +
> +    this.jsonFile.data.listeners[broadcastId] = {version, sourceInfo};

Would we ever want to have multiple listeners for the same broadcast ID, or to remove a listener for an existing ID? (If we ever move or refactor a module).

::: dom/push/PushBroadcastService.jsm:108
(Diff revision 4)
> +      const version = data.broadcasts[broadcastId];
> +      if (version === DUMMY_VERSION_STRING) {
> +        console.info("Ignoring", version, "because it's the dummy version");
> +        continue;
> +      }
> +      // We don't know this broadcastID. This is probably a bug?

Could be a bug, but could also be our version of Firefox is older, and doesn't know about this ID yet, or we switched to a different ID in a newer version, and sending both for back-compat...

::: dom/push/PushBroadcastService.jsm:151
(Diff revision 4)
> +        continue;
> +      }
> +
> +      try {
> +        await handler.receivedBroadcastMessage(version, broadcastId);
> +      } catch(e) {

Micro-nit: Space after `catch`.

::: dom/push/PushBroadcastService.jsm:160
(Diff revision 4)
> +      }
> +
> +      // Broadcast message applied successfully. Update the version we
> +      // received if it's different than the one we had.  We don't
> +      // enforce an ordering here (i.e. we use != instead of <)
> +      // because we don't know what the ordering of the service's

So versions are mostly opaque, right? Do we need to worry about a race where we've been offline for a while, and, on reconnect, the server sends us the new version before the old? Or will the server collapse messages so we only get one (broadcast ID, version) tuple, or, if not, are we guaranteed to always receive broadcast messages in order?

::: dom/push/PushBroadcastService.jsm:179
(Diff revision 4)
> +
> +function initializeBroadcastService() {
> +  // Fallback path for xpcshell tests.
> +  let path = "broadcast-listeners.json";
> +  if (OS.Constants.Path.profileDir) {
> +    // Real path for use in a real profile.

Is this missing in xpcshell, even if you call `do_get_profile()` in `run_test`?

::: dom/push/PushService.jsm:769
(Diff revision 4)
>  
>    /**
> +   * Dispatches a broadcast notification to the BroadcastService.
> +   */
> +  receivedBroadcastMessage(message) {
> +    pushBroadcastService.receivedBroadcastMessage(message);

Let's add a `catch` with a `console.error` here, too. (Maybe this is why you weren't seeing `Cu.import` errors? I think our tests have unhandled rejection tracking, but not certain, and not sure about production code).

::: dom/push/PushServiceWebSocket.jsm:594
(Diff revision 4)
> +      // The reply isn't technically a broadcast message, but it has
> +      // the shape of a broadcast message (it has a broadcasts field).
> +      this._mainPushService.receivedBroadcastMessage(reply);
> +    }
> +
>      // By this point we've got a UAID from the server that we are ready to

A few things:

* I think the `reply.broadcasts` check might belong after this block, after we've checked that the UAID is up-to-date.
* What should we do if the UAID changes? I'm guessing broadcast isn't currently UAID-dependent (would it ever be in the future, if we, say, want to send a message to a subset of Firefox users?), so is it OK to leave all subscriptions in place? Or should we invalidate all existing broadcast subscriptions, and make everyone resubscribe?
* If we want to invalidate, will we want another function on the broadcast handler, like `broadcastSubscriptionLost`, to notify services to resubscribe? Or should we keep things simple and just wait until the next time they call `addListener`?
Attachment #8967018 - Flags: review?(kit)
Comment on attachment 8967016 [details]
Bug 1440022: Some plausible test cases

https://reviewboard.mozilla.org/r/235688/#review245878

This looks great, holding off on official r+ until we figure out if we need any API changes in part 3.

::: dom/push/test/xpcshell/xpcshell.ini:5
(Diff revision 4)
>  [DEFAULT]
>  head = head.js head-http2.js
>  # Push notifications and alarms are currently disabled on Android.
>  skip-if = toolkit == 'android'
> +support-files = broadcast_handler.jsm

I was under the impression you only need `EXTRA_JS_MODULES`, but is this necessary, too?
Attachment #8967016 - Flags: review?(kit)
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review248364

> Needed by a hello message?

Oops, yes, thanks.

> You can also use the `dataPostProcessor` option to `JSONFile` to do this, if you want, as in https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/browser/extensions/formautofill/FormAutofillStorage.jsm#1784,1791.
> 
> We might also want to add a `version` field to the data, in case we change the storage schema later. I'm not sure what we should do if we get a version newer than what we know about (in the case of a profile downgrade, for example)...probably downgrade the version, but leave the source infos in place, so that we rerun migration code on the next upgrade.
> 
> This is all hypothetical, because we don't know how we'll evolve the schema yet, if at all. But it's good to have something in place.

OK, I set a version field which is now unconditionally 1.

> Let's validate that both are strings, too.

Good idea, thanks.

> Nit: What do you think of `symbol` or `symbolName`, instead of `attributeName`? (Just because `XPCOMUtils.jsm` calls this a "symbol", and to match `EXPORTED_SYMBOLS`).

No strong opinion, I changed it.

> Please document these params in the JSDoc comment, too.

OK, done.

> Would we ever want to have multiple listeners for the same broadcast ID, or to remove a listener for an existing ID? (If we ever move or refactor a module).

I kept the API simple for now because I didn't have a concrete use case for either of those and figured we could add them later. Specifically:

- If we want to have multiple listeners for a single broadcast ID, probably we could define a listener that calls the other listeners. So if we want to update both addons and blocklists on message X, we can define an ad hoc message X handler that calls addons.update() and blocklists.update().
- If we no longer want to listen to a message (i.e. remove a handler), then (as :rnewman points out), there isn't really a call site from which to issue a `removeListener` call. He proposed building a migration mechanism to handle these cases, and I think we can build that later once we know what the case actually is.
- If we move the code that handles messages for a broadcast ID, we could just call `addListener` again (from some updater code that gets scheduled outside of PushBroadcastService), or we could again build a migration mechanism.

> Could be a bug, but could also be our version of Firefox is older, and doesn't know about this ID yet, or we switched to a different ID in a newer version, and sending both for back-compat...

For us to hit this code path, we have to have received a message which isn't present in the `listeners` hash. Because it isn't in the `listeners` hash, we never told the server we wanted updates about it, and it wasn't in the file when we loaded it and it isn't in our build. It's not enough that we sent both to megaphone for backwards compat, but that the megaphone server gave us both of them without our asking for them. So I don't think any of your scenarios are relevant to this line of code. It's really more of a defensive programming safety net then any situation I can imagine hitting.

> Micro-nit: Space after `catch`.

OK, got it, thanks.

> So versions are mostly opaque, right? Do we need to worry about a race where we've been offline for a while, and, on reconnect, the server sends us the new version before the old? Or will the server collapse messages so we only get one (broadcast ID, version) tuple, or, if not, are we guaranteed to always receive broadcast messages in order?

Megaphone doesn't really think in terms of "messages". It's more structured around "most recent version". When a client says hello or subscribes to some broadcastID, Megaphone compares what the client gave it vs. what it has (which must be the newest version). When it sends a notification to the client, it's the newest version it knows about. So I guess this corresponds most closely to what you described as "collapses messages". I guess it's possible that Megaphone's information is outdated, and the client (responding to an update at T1) fetches everything at the server (which includes stuff at T2) before Megaphone gets the memo that there's T2 stuff too. I haven't thought it through carefully, but I would have assumed that handling this would be the responsibility of the consumer.

> Is this missing in xpcshell, even if you call `do_get_profile()` in `run_test`?

Like I wrote in https://reviewboard.mozilla.org/r/235692/#comment312644, yes, this is in fact necessary. Otherwise, you get a failure at import time:

```
0:00.57 ERROR TypeError: invalid path component at resource://gre/modules/osfile/ospath_unix.jsm:90
join@resource://gre/modules/osfile/ospath_unix.jsm:90:13
initializeBroadcastService@resource://gre/modules/PushBroadcastService.jsm:198:14
@resource://gre/modules/PushBroadcastService.jsm:202:28
@/home/ethan/Jobs/Mozilla/gecko-git/obj-x86_64-pc-linux-gnu/_tests/xpcshell/dom/push/test/xpcshell/head.js:19:33
load_file@/home/ethan/Jobs/Mozilla/gecko-git/testing/xpcshell/head.js:618:7
_load_files@/home/ethan/Jobs/Mozilla/gecko-git/testing/xpcshell/head.js:630:3
_execute_test@/home/ethan/Jobs/Mozilla/gecko-git/testing/xpcshell/head.js:495:3
@-e:1:1
```

... because we set up a singleton instance at import time. Like I said, I'm not convinced this is the best approach or even functions correctly in production.

> Let's add a `catch` with a `console.error` here, too. (Maybe this is why you weren't seeing `Cu.import` errors? I think our tests have unhandled rejection tracking, but not certain, and not sure about production code).

OK, I did, thanks.

> A few things:
> 
> * I think the `reply.broadcasts` check might belong after this block, after we've checked that the UAID is up-to-date.
> * What should we do if the UAID changes? I'm guessing broadcast isn't currently UAID-dependent (would it ever be in the future, if we, say, want to send a message to a subset of Firefox users?), so is it OK to leave all subscriptions in place? Or should we invalidate all existing broadcast subscriptions, and make everyone resubscribe?
> * If we want to invalidate, will we want another function on the broadcast handler, like `broadcastSubscriptionLost`, to notify services to resubscribe? Or should we keep things simple and just wait until the next time they call `addListener`?

As far as I can tell, broadcast stuff isn't in any way affected by getting a new UAID. Because Megaphone only tracks the "most current version" for each broadcastID, there's no client state on the server that we need to react to the loss of, so no reason for consumers to have to take any action. We've already sent, as part of our `hello`, the complete information about the set of broadcastIDs that we understand, so there's no reason to resend anything either.

I guess the design of Megaphone precludes the sending of messages to a subset of users. But even if it didn't, from the client's perspective, nothing has changed when the UAID changes -- it's still waiting to receive the same messages and they will still be handled in the same way.

It sounds like you're trying to handle a case where a consumer gets "disconnected". I don't think that something like that is in scope for global broadcast. Global broadcast lets us find out that upstream data has changed, but that change is always unrelated to any specific client.

I moved the `reply.broadcasts` check into `finishHandshake`.
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review248690


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/blocklist-clients.js:193
(Diff revision 2)
> +
> +    const currentVersion = await client.openCollection(async collection => {
> +      return collection.db.getLastModified();
> +    });
> +    const moduleInfo = {
> +      moduleURI: __URI__,

Error: '__uri__' is not defined. [eslint: no-undef]
Comment on attachment 8967016 [details]
Bug 1440022: Some plausible test cases

https://reviewboard.mozilla.org/r/235688/#review245878

> I was under the impression you only need `EXTRA_JS_MODULES`, but is this necessary, too?

I wasn't able to import the file without this.
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review248360

> Is `initialize` safe to call multiple times, if a non-broadcast consumer calls it first?

Oops, it wasn't, thanks!
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review253030

::: dom/push/PushService.jsm:245
(Diff revision 5)
>        this._service.disconnect();
>      }
>  
>      let records = await this.getAllUnexpired();
> +    let broadcastListeners = await pushBroadcastService.getListeners();
>  

I think there's a potential race here. Consider the following sequence of events:

- await pushBroadcastService.getListeners();
- Control returns to the event loop.
- Some new component calls PushService.subscribeBroadcast(), which is discarded (since the stat is not PUSH_SERVICE_RUNNING)
- getListeners()'s promise resolves, without the component that tried to subscribe
- State is set to PUSH_SERVICE_RUNNING, and things continue as normal.

I haven't dug all the way down to the bottom of this to prove that a race can happen right now, but the order of events combined with async handling here is a red flag that warrants investigation (and, if it's safe, probably a comment here explaining why it's safe).
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review253030

> I think there's a potential race here. Consider the following sequence of events:
> 
> - await pushBroadcastService.getListeners();
> - Control returns to the event loop.
> - Some new component calls PushService.subscribeBroadcast(), which is discarded (since the stat is not PUSH_SERVICE_RUNNING)
> - getListeners()'s promise resolves, without the component that tried to subscribe
> - State is set to PUSH_SERVICE_RUNNING, and things continue as normal.
> 
> I haven't dug all the way down to the bottom of this to prove that a race can happen right now, but the order of events combined with async handling here is a red flag that warrants investigation (and, if it's safe, probably a comment here explaining why it's safe).

I think I'm OK here, although it's not exactly elegant:

- The only caller for subscribeBroadcast() is (by design) PushBroadcastService#addListener.
- PushBroadcastService#addListener waits on the same promise before calling PushService#subscribeBroadcast.
- If PushBroadcastService#addListener gets woken up first, then it gets a chance to update the listeners before they are returned to PushService.
- If PushService gets woken up first, it immediately sets its state to RUNNING, meaning that when PushBroadcastService wakes up, it can call subscribeBroadcast correctly (it will queue a message, to be delivered after the socket is set up).

I've made a couple changes to try to make this clearer (reordered some operations in PushBroadcastService#addListener and added some comments). I admit that what I have isn't great. I don't really have a lot of experience with this kind of multithreading and I'm not really sure how to improve it. Here are some ideas:

- Always send the subscribe_broadcast message on the socket. They'll only get sent after the socket is set up anyhow, so the worst that could happen is we subscribe to something we're already subscribed to, in which case we might get a spurious broadcast message, which is probably fine.
- Add to PushService a queue of subscriptions that should be processed once we move to STATE_RUNNING, with any that are already present in the listeners being dropped.

Neither of these feel really elegant either. If you have a suggestion for how to handle this better, I'd love to hear it!
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review255638


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/blocklist-clients.js:268
(Diff revision 3)
> +
> +    const currentVersion = await client.openCollection(async collection => {
> +      return collection.db.getLastModified();
> +    });
> +    const moduleInfo = {
> +      moduleURI: __URI__,

Error: '__uri__' is not defined. [eslint: no-undef]
In case I haven't already mentioned it somewhere, the fourth patch was meant to be a combination prototype/demonstration of how this API might get used in reality. However, having talked to :leplatrem, we agreed that this kind of per-setting hook-up isn't really optimal, and I have a clearer design in mind, which I'm going to implement now. I'm going to leave the fourth patch in place for now in case it's helpful to see it "in action" while I work on the correct approach.
Comment on attachment 8967016 [details]
Bug 1440022: Some plausible test cases

https://reviewboard.mozilla.org/r/235688/#review256036
Attachment #8967016 - Flags: review?(kit) → review+
Comment on attachment 8967018 [details]
Bug 1440022: initial implementation

https://reviewboard.mozilla.org/r/235692/#review256038

Thanks so much, Ethan, this all looks great! I'm really sorry for the review delay, I wasn't sure what the timeline was, and should have asked earlier. Land at your convenience! \o/

::: commit-message-b8fc6:1
(Diff revision 7)
> +Bug 1440022: initial proof-of-concept r?kitcambridge

Let's update the commit message to reflect that it's not a proof-of-concept anymore. \o/

::: dom/push/PushBroadcastService.jsm:15
(Diff revision 7)
> +ChromeUtils.defineModuleGetter(this, "JSONFile", "resource://gre/modules/JSONFile.jsm");
> +
> +var EXPORTED_SYMBOLS = ["pushBroadcastService"];
> +
> +// We are supposed to ignore any updates with this version.
> +// FIXME: what is the actual "dummy" version?

Please file a follow-up bug for this, and mention it in the comment.

::: dom/push/PushBroadcastService.jsm:30
(Diff revision 7)
> +ChromeUtils.defineModuleGetter(this, "PushService", "resource://gre/modules/PushService.jsm");
> +
> +class InvalidSourceInfo extends Error {
> +  constructor(message) {
> +    super(message);
> +    this.message = 'InvalidSourceInfo';

Nit: `this.name`?
Attachment #8967018 - Flags: review?(kit) → review+
See Also: → 1467550
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review256364

Still looks good!

::: services/settings/remote-settings.js:5
(Diff revision 5)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global __URI__ */

How about adding `__URI__` to https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/jsm.js, so that other modules can use it without ESLint complaining, too? (I *think* that's the only change you'll need to make for that; if it doesn't work, don't worry about it).

::: services/settings/remote-settings.js:674
(Diff revision 5)
>  
> +
> +  const broadcastID = "remote-settings/monitor_changes";
> +  let currentVersion = "";
> +  if (Services.prefs.prefHasUserValue(PREF_SETTINGS_LAST_ETAG)) {
> +    currentVersion = Services.prefs.getCharPref(PREF_SETTINGS_LAST_ETAG);

`getStringPref` is the new hotness. You can also do `Services.prefs.getStringValue(PREF_SETTINGS_LAST_ETAG, "")`, and remove the `prefHasUserValue` check.

::: services/settings/remote-settings.js:680
(Diff revision 5)
> +  }
> +  const moduleInfo = {
> +    moduleURI: __URI__,
> +    symbolName: "remoteSettingsBroadcastHandler",
> +  };
> +  pushBroadcastService.addListener(broadcastID, currentVersion,

The `ETag` is a quoted string, which Megaphone might reject. I asked about specifically allowing this in https://github.com/mozilla-services/megaphone/pull/45, so we don't have to de-quote or hash it.
Comment on attachment 8974122 [details]
Bug 1440022: hook up remote-settings to broadcast messages

https://reviewboard.mozilla.org/r/242412/#review256364

> How about adding `__URI__` to https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/jsm.js, so that other modules can use it without ESLint complaining, too? (I *think* that's the only change you'll need to make for that; if it doesn't work, don't worry about it).

I tried and it didn't work. Dropping..

> `getStringPref` is the new hotness. You can also do `Services.prefs.getStringValue(PREF_SETTINGS_LAST_ETAG, "")`, and remove the `prefHasUserValue` check.

Got it, thanks!

> The `ETag` is a quoted string, which Megaphone might reject. I asked about specifically allowing this in https://github.com/mozilla-services/megaphone/pull/45, so we don't have to de-quote or hash it.

It seems like the consensus is that an ETag is fine.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b7d5b0db60ad
Some plausible test cases r=lina
https://hg.mozilla.org/integration/autoland/rev/00cf71bb3861
Some infrastructure for testing broadcast_subscribe r=lina
https://hg.mozilla.org/integration/autoland/rev/4f84a6745a51
initial implementation r=lina
https://hg.mozilla.org/integration/autoland/rev/9fa127e6df56
hook up remote-settings to broadcast messages r=lina
Keywords: checkin-needed
(In reply to Ethan Glasser-Camp (:glasserc) from comment #56)
> > `getStringPref` is the new hotness. You can also do `Services.prefs.getStringValue(PREF_SETTINGS_LAST_ETAG, "")`, and remove the `prefHasUserValue` check.

Eek, sorry, I meant `getStringPref`.

(In reply to Bogdan Tara[:bogdan_tara] from comment #62)
> Backed out 4 changesets (bug 1440022) for nsSocketTransport crashes when
> trying to access the internet.

Hmm, right. Sorry I didn't catch this. Unconditionally connecting to the push server is going to hit this whenever a module accesses `nsIPushService`.

We could either disable the connection by default in testing (`dom.push.connection.enabled`), or only connect if we have broadcast subscriptions *or* records. I don't think `RemoteSettings` is wired up in xpcshell, so that could work...though, disabling by default might be better, because other tests that initialize the broadcast service in the future would still run into this.

Let's chat during the work week?
Comment on attachment 8985289 [details]
Bug 1440022: disconnect push tests from actually running

https://reviewboard.mozilla.org/r/250904/#review257184
Attachment #8985289 - Flags: review?(kit) → review+
I just realized that if we reregister with the same broadcast ID we can have two different version IDs. In principle the answer is "don't do that" but it can happen if e.g. the update handler is buggy. What should we do here?
Flags: needinfo?(eglassercamp)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s aa7585c68eecadb2e8b53333467fa239dbe5b920 -d 94e0de0c19d1: rebasing 468391:aa7585c68eec "Bug 1440022: disconnect push tests from actually running r=lina"
rebasing 468392:43837e883d7e "Bug 1440022: Some plausible test cases r=lina"
merging dom/push/test/xpcshell/head.js
merging dom/push/test/xpcshell/moz.build
merging dom/push/test/xpcshell/xpcshell.ini
rebasing 468393:3c7083d445af "Bug 1440022: Some infrastructure for testing broadcast_subscribe r=lina"
merging dom/push/test/xpcshell/head.js
rebasing 468394:3b9a7143aa66 "Bug 1440022: initial implementation r=lina"
merging dom/push/PushService.jsm
merging dom/push/PushServiceAndroidGCM.jsm
merging dom/push/PushServiceHttp2.jsm
merging dom/push/PushServiceWebSocket.jsm
merging dom/push/moz.build
warning: conflicts while merging dom/push/PushService.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Tried to land but got the conflict above.
Flags: needinfo?(eglassercamp)
I rebased everything. Please try again.
Flags: needinfo?(eglassercamp)
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b19a1ccabd89
disconnect push tests from actually running r=lina
https://hg.mozilla.org/integration/autoland/rev/5988107687ce
Some plausible test cases r=lina
https://hg.mozilla.org/integration/autoland/rev/0c6883fcf169
Some infrastructure for testing broadcast_subscribe r=lina
https://hg.mozilla.org/integration/autoland/rev/4b750a5cd250
initial implementation r=lina
https://hg.mozilla.org/integration/autoland/rev/5402f880b427
hook up remote-settings to broadcast messages r=lina
Keywords: checkin-needed
See Also: → 1472238
You need to log in before you can comment on or make changes to this bug.