Closed Bug 1442133 Opened 6 years ago Closed 6 years ago

FxA Messages client implementation

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

Details

Attachments

(1 file)

Once we know the format of a "send tab" payload (bug 1442123)m can encrypt it (bug 1442128) and know the server to hit (bug 1442131), we can request an oath token for the server (token scopes and server end-points are defined in https://docs.google.com/document/d/1YT6gh125Tu03eM42Vb_LKjvgxc4qrGGZsty1_ajf2YM/edit#) and make the POST!

There's a bit of devil in the detail here as the exact format of the POST payload is TBD, but the above doc does nail most of it down.
Note that comment 0 is incorrect:

> and know the server to hit (bug 1442131), we can request an oath token for the server
> (token scopes and server end-points are defined in
> https://docs.google.com/document/d/1YT6gh125Tu03eM42Vb_LKjvgxc4qrGGZsty1_ajf2YM/edit#)
> and make the POST

This POST is made to the FxA server, which takes responsibility for hitting the pushbox server and sending the push. IIUC, https://github.com/mozilla/fxa-auth-server/issues/2318 implies it might be the exact same endpoint we hit now for collection-changed notifications. Thus, knowledge of what the pushbox server is and getting oauth tokens isn't necessary for this bug, but rather for bug 1442135.
> it might be the exact same endpoint we hit now for collection-changed notifications

Yep, I think given the final shape of the rest of the proposal, re-using the existing /notify endpoint is reasonable.
Priority: -- → P2
Assignee: nobody → eoger
This is not really working yet (pushbox server still has bug that needs to be worked out), but I am very close to having an end-to-end POC running on my machine.
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review232242

Nice work Ed. We should think a little more about where the "boundaries" are (in terms of who exposes what etc) for this, but your XXX comments show you are already thinking about this a little. But all in all, this seems like it is in great shape fir a first pass.

::: browser/base/content/browser-sync.js:324
(Diff revision 1)
>      let url = this.PRODUCT_INFO_BASE_URL;
>      url += "send-tabs/?utm_source=" + Services.appinfo.name.toLowerCase();
>      switchToTabHavingURI(url, true, { replaceQueryString: true });
>    },
>  
> -  sendTabToDevice(url, clientId, title) {
> +  isPushboxAwareDevice(deviceId) {

This seems like it should probably be in the pushboxService

::: services/fxaccounts/FxAccountsConfig.jsm:75
(Diff revision 1)
>    async promiseConnectDeviceURI(entrypoint) {
>      return this._buildURL("connect_another_device", {entrypoint}, true);
>    },
>  
> +  async promisePushboxURI() {
> +    if (PUSHBOX_URL &&

Is this temporary? I hope so :) I think we should treat the pushbox url like we treat any other URL used by FxA (ie, if we really need to inspect cache-control headers, we should do it for every url - in which case a different bug might make sense).

It's complicated by the fact that this is a "new" pref/config var, but I think that's still solvable (eg, having no user-value for our pushbox URL but a user-value for the config url is a good indication we need to re-fetch the config.

::: services/pushbox/PushboxClient.js:18
(Diff revision 1)
> +  constructor(options = {}) {
> +    this.fxaConfig = options.fxaConfig || FxAccounts.config;
> +    this.fxAccounts = options.fxAccounts || fxAccounts;
> +  }
> +
> +  async getMessagesFor(service, index = null) {

maybe better named as getMessagesSince or similar?

::: services/pushbox/PushboxClient.js:28
(Diff revision 1)
> +    const url = await this._buildURL(service, uid, deviceId, index);
> +    const token = await this._oAuthTokenForDevice(deviceId);
> +    const response = await fetch(url, {
> +      headers: {"Authorization": `Bearer ${token}`}
> +    });
> +    if (!response.ok) {

We probably want better logging here. I imagine we should also have a followup bug to handle backoffs etc.

::: services/pushbox/PushboxMessageHandler.js:3
(Diff revision 1)
> +/* 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/. */

TBH I'm not quite convinced this needs its own module (although I don't feel that strongly about it)

Another pattern to consider might be that external code can register to handle a service passing a callback in, meaning new services could theoretically be handled without changing any of this code.

::: services/pushbox/PushboxService.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

The use of "Service" in the name seems a little misleading - typically that means a singleton.

Indeed, it might make more sense for it to be a singleton so code that wants to use pushbox needn't go via the FxA module (eg, imagine a future where your rust impl of FxA is used in desktop, we'd probably want to decouple things like this whereever we can).

::: services/pushbox/PushboxService.js:15
(Diff revision 1)
> +const {PushCrypto} = ChromeUtils.import("resource://gre/modules/PushCrypto.jsm", {});
> +ChromeUtils.import("resource://gre/modules/PushboxMessageHandler.js");
> +const {setInterval, clearInterval} = ChromeUtils.import("resource://gre/modules/Timer.jsm", {});
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +const POLLING_INTERVAL = 5 * 60 * 1000; // in ms.

I imagine this will end up being a pref?

::: services/pushbox/PushboxService.js:25
(Diff revision 1)
> +    this.pushCrypto = options.pushCrypto || PushCrypto;
> +    this.client = options.client || new PushboxClient();
> +    this.handler = options.handler || new PushboxMessageHandler();
> +    this.fxAccounts = options.fxAccounts || fxAccounts;
> +    this.indexesBranch = Services.prefs.getBranch(BRANCH_INDEXES);
> +    // TODO: Need to figure out who should build PushboxService and call this

If this module was a singleton, it could make sense for Sync itself to call .startPolling (or maybe with a name like .ensureSomething()) so that missed messages come in ASAP. Any future consumers of pushbox could then do the same (meaning it would need to be safe to call multiple times etc)

Another (probably bad) idea might be to have Sync schedule this polling - IOW, make this a light-weight sync engine) - then we get "sync now", better handling of offline and sleep/resume, etc, and a logging infrastructure that will help make diagnosis easier. We might be able to do it purely via observers, especially if we invent a way to get the promise back to Sync itself (ie, so sync knows when this external thing has finished)

Dunno :)

::: services/pushbox/PushboxService.js:33
(Diff revision 1)
> +    this.startPolling();
> +  }
> +
> +  finalize() {
> +    clearInterval(this.intervalId);
> +    this.indexesBranch.resetBranch("");

doesn't this mean we'll start again on the next browser session, replaying all old messages?

I also wonder if we should be listening to events from FxA about our device ID changing - I believe that if that ever happens for whatever reason we will want to consider resetting this (eg, disconnecting and reconnecting sync will almost certainly make the old indexes invalid)

::: services/pushbox/PushboxService.js:66
(Diff revision 1)
> +    };
> +    return this._send(PushboxConstants.SERVICES.SEND_TAB, to, payload);
> +  }
> +
> +  // TODO: needs a better name that implies flush unread + handle messages.
> +  async consumeRemoteMessages(service) {

I think consumePendingRemoteMessages is good enough (but I'm sure there's a better one somewhere...)

::: services/pushbox/PushboxService.js:69
(Diff revision 1)
> +
> +  // TODO: needs a better name that implies flush unread + handle messages.
> +  async consumeRemoteMessages(service) {
> +    log.info("Retrieving unread messages.");
> +    const index = this._getIndexForService(service);
> +    // TODO: recover if we have sent an erroneous token and the server tells us

yeah, this worries me a little. What does such recovery look like? Hopefully not "set index=null" as IIUC, that means the last 30 days or so of pushes will be resent.

::: services/pushbox/PushboxService.js:72
(Diff revision 1)
> +    log.info("Retrieving unread messages.");
> +    const index = this._getIndexForService(service);
> +    // TODO: recover if we have sent an erroneous token and the server tells us
> +    // that.
> +    const {index: newIndex, messages} = await this.client.getMessagesFor(service, index);
> +    this._setIndexForService(service, newIndex);

ISTM this should be after we've called the handler, so if we happen to be shutting down etc we are more likely to re-fetch it next startup?

::: services/pushbox/PushboxService.js:75
(Diff revision 1)
> +    // that.
> +    const {index: newIndex, messages} = await this.client.getMessagesFor(service, index);
> +    this._setIndexForService(service, newIndex);
> +    const payloads = [];
> +    for (const message of messages) {
> +      const decrypted = await this._decrypt(message.data);

We should probably try/catch here.

::: services/pushbox/PushboxService.js:121
(Diff revision 1)
> +  async _send(service, to = null, payload) {
> +    // TODO:
> +    // This will make a "full" HTTP request EVERY time.
> +    // We should at least eTag these requests, or even better, store the list
> +    // of FxA Devices "globally" and just pass to this function full devices
> +    // objects.

I agree that getDeviceList() should get smarter. FxA could also grow a getDeviceById() type function that could help with those smarts - if we have the device ID cached and not very stale, we just return it. If it doesn't exist, we refresh and try again.
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review232246

(oh, I was very inconsistent with when I opened an issue vs when I didn't, so don't read too much into that)
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review232242

> doesn't this mean we'll start again on the next browser session, replaying all old messages?
> 
> I also wonder if we should be listening to events from FxA about our device ID changing - I believe that if that ever happens for whatever reason we will want to consider resetting this (eg, disconnecting and reconnecting sync will almost certainly make the old indexes invalid)

abortExistingFlow() (who calls finalize()) is called by two methods: signOutLocal and setSignedInUser, which itself is only called by the WebChannel login message. We won't clear that pref on browser shutdown/startup.
I agree with you, we should listen to observer notifications, especially if we end up as a singleton service as you are mentionning.

> yeah, this worries me a little. What does such recovery look like? Hopefully not "set index=null" as IIUC, that means the last 30 days or so of pushes will be resent.

It's almost as the never needs some kind of mechanism to mark all retrieved messages as "read" ;)
Oops, I meant "server" in the last sentence of comment 7.
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review232242

> Is this temporary? I hope so :) I think we should treat the pushbox url like we treat any other URL used by FxA (ie, if we really need to inspect cache-control headers, we should do it for every url - in which case a different bug might make sense).
> 
> It's complicated by the fact that this is a "new" pref/config var, but I think that's still solvable (eg, having no user-value for our pushbox URL but a user-value for the config url is a good indication we need to re-fetch the config.

I think for this patch we should simply do what we've done for the other URLs in past:
Ship the official pushbox URL in a pref by default, and let the AutoConfig replace that URL if the autoconfig pref is set.

However, in the near future, I'd like to see these server URLs being driven by the AutoConfig system alone (aka we only ship the "identity.fxaccounts.remote.root" pref and FxAccountsConfig figures out the other URLs by querying .well-known/fxa-client-configuration).
This is a bit more complicated that what I'm describing however, since some users self-host their own sync server but rely on the officials FxA servers for authentication purposes (in that case "identity.sync.tokenserver.uri" would be incorrect).
How about we file a bug to think about all of this?
Just posting the patch really quick if someone needs to try the current POC.
This patch doesn't reflect the all review comments above yet!
Ready for review! This is testable against https://pushbox2.dev.lcip.org.
Notable changes:
- Encryption/decryption, thanks to Mark's work in bug 1442128.
- Pushbox code folded into FxAccounts since we don't talk to the Pushbox server directly anymore.
Summary: POST pushbox payloads → FxA Messages client implementation
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review235712

This is looking great - nice work Ed! I wouldn't mind seeing it again, and I think I'll flag Thom to have a look too, as a fresh set of eyes never hurts.

One other followup I think we need is telemetry - we are going to want to know what the send and receive rates are for this stuff, but I think it's fine to do that in a followup (although that will be a little messy as telemetry is currently tied tightly to Sync - we probably want to loosen that a little and make it "sync + fxa")

::: browser/app/profile/firefox.js:1414
(Diff revision 3)
>  
>  // The remote URL of the FxA OAuth Server
>  pref("identity.fxaccounts.remote.oauth.uri", "https://oauth.accounts.firefox.com/v1");
>  
> +// Polling interval for the messages service.
> +pref("identity.fxaccounts.messages.pollingInterval", 300); // 5 minutes.

What do you think about my previous idea of doing this at sync time? That would mean we get things like offline detection and post-wake etc for free.

(regardless, 5 minutes seems a little too agressive given this is really just a fallback for push which we expect to work well)

::: services/fxaccounts/FxAccounts.jsm:69
(Diff revision 3)
>    "hasLocalSession",
>    "invalidateCertificate",
> +  "isMessagesAwareDevice",
>    "loadAndPoll",
>    "localtimeOffsetMsec",
> +  "messages",

I don't understand why |messages| is public, but getMessages, isMessagesAwareDevice and storeMessages aren't on that object?

::: services/fxaccounts/FxAccounts.jsm:399
(Diff revision 3)
>    VERIFICATION_POLL_START_SLOWDOWN_THRESHOLD: 5,
>    // The current version of the device registration, we use this to re-register
>    // devices after we update what we send on device registration.
> -  DEVICE_REGISTRATION_VERSION: 2,
> +  DEVICE_REGISTRATION_VERSION: 3,
> +
> +  // Our current device capabilities that we report to Firefox Accounts.

I think it's worth adding an extra line here noting that changing the capabilities means you must also bump DEVICE_REGISTRATION_VERSION.

::: services/fxaccounts/FxAccounts.jsm:400
(Diff revision 3)
>    // The current version of the device registration, we use this to re-register
>    // devices after we update what we send on device registration.
> -  DEVICE_REGISTRATION_VERSION: 2,
> +  DEVICE_REGISTRATION_VERSION: 3,
> +
> +  // Our current device capabilities that we report to Firefox Accounts.
> +  CURRENT_CAPABILITIES: [CAPABILITY_MESSAGES],

nit: the word "CURRENT" might imply something other than "current device" - maybe DEVICE_CAPABILITIES or SUPPORTED_CAPABILITIES or similar? (but I don't care too much)

::: services/fxaccounts/FxAccounts.jsm:410
(Diff revision 3)
>    // as it's called after this object has been mocked for tests.
>    initialize() {
>      this.currentTimer = null;
>      this.currentAccountState = this.newAccountState();
> +
> +    // Start polling once we're done initializing.

even though I think we should consider not polling in this way (see above) it seems cleaner to just do this.messages.initialize() to keep any polling etc as impl details of |messages|

::: services/fxaccounts/FxAccounts.jsm:444
(Diff revision 3)
> +      this._messages = new FxAccountsMessages();
> +    }
> +    return this._messages;
> +  },
> +
> +  isMessagesAwareDevice(device) {

I think this would parse better as isDeviceMessagesAware() or similar.

On the other hand though, it seems painful to add a new method every time we add a new capability - another option might be to have a public object called, say, CAPABILITIES, so we end up with a more generic method that callers could use like:

`fxAccounts.deviceHasCapability(fxAccounts.CAPABILITIES.MESSAGES)`

(or similar) - WDYT?

::: services/fxaccounts/FxAccounts.jsm:463
(Diff revision 3)
> +    const opts = {};
> +    if (messagesIndexes && messagesIndexes[service] !== undefined) {
> +      opts.index = messagesIndexes[service];
> +    }
> +    const {index: newIndex, messages} = await this.fxAccountsClient.retrieveMessages(sessionToken, service, opts);
> +    await this.currentAccountState.updateUserAccountData({

I think these should probably be associated with the device - eg, IIUC, should `_recoverFromUnknownDevice` be called these need to be reset.

The easiest option would be to change that function, but another alternative we could consider is to slightly change how devices are handled in storage - instead of what we do now:
`
this.currentAccountState.updateUserAccountData({
        deviceId: device.id,
        deviceRegistrationVersion: this.DEVICE_REGISTRATION_VERSION
      })
`     

and `_recoverFromUnknownDevice` doing:

`this.currentAccountState.updateUserAccountData({ deviceId: null })`

we could consider storing an object with `device: {id, registrationVersion` and set *that* to null on an unknown device. You could then store any per-device state, such as your message indexes, safe in the knowledge that it all gets dropped without remembering to manage each item individually.

Similarly for `_recoverFromDeviceSessionConflict` where it appears like we'd probably want to reset if the device ID we find isn't what we think it is (which I expect is all the time, or we wouldn't end up there in the first place)

It's probably a bit unfair to block this patch landing on this, but if you agree that's a reasonable idea, a followup would be fine (but this patch would need to reset them in `_recoverFromUnknownDevice` and possibly `_recoverFromDeviceSessionConflict`

::: services/fxaccounts/FxAccountsClient.jsm:449
(Diff revision 3)
>      return this._request("/account/devices/notify", "POST",
>        deriveHawkCredentials(sessionTokenHex, "sessionToken"), body);
>    },
>  
>    /**
> +   * Stores a message in the recipient's message box.

This comment is wrong.

::: services/fxaccounts/FxAccountsCommon.js:219
(Diff revision 3)
>  
>  // The fields we save in the plaintext JSON.
>  // See bug 1013064 comments 23-25 for why the sessionToken is "safe"
>  exports.FXA_PWDMGR_PLAINTEXT_FIELDS = new Set(
>    ["email", "verified", "authAt", "sessionToken", "uid", "oauthTokens", "profile",
> -  "deviceId", "deviceRegistrationVersion", "profileCache"]);
> +  "deviceId", "deviceRegistrationVersion", "profileCache", "messagesIndexes"]);

I think `messageIndexes` is better - the double-plural seems wrong.

::: services/fxaccounts/FxAccountsMessages.js:10
(Diff revision 3)
> +const EXPORTED_SYMBOLS = ["FxAccountsMessages", "FxAccountsMessagesHandler"];
> +
> +ChromeUtils.import("resource://gre/modules/FxAccounts.jsm");
> +ChromeUtils.import("resource://gre/modules/FxAccountsCommon.js");
> +ChromeUtils.import("resource://gre/modules/Preferences.jsm");
> +const {PushCrypto} = ChromeUtils.import("resource://gre/modules/PushCrypto.jsm", {});

ISTM that some of these should be lazy (ie, stuff that isn't used by startPolling/initialize)

::: services/fxaccounts/FxAccountsMessages.js:123
(Diff revision 3)
> +      }
> +    }
> +  }
> +
> +  async _encrypt(payload, device, encoder) {
> +    payload = encoder.encode(JSON.stringify(payload));

it seems a little odd to pass payload and encoder in this way - maybe have the caller to the .encode and .stringify?

::: services/fxaccounts/FxAccountsMessages.js:146
(Diff revision 3)
> +    // Might want to batch together some events, hence payloadS.
> +    switch (service) {
> +      case SERVICES.SEND_TAB:
> +        return this._handleSendTabPayloads(payloads);
> +      default:
> +        throw new Error("Unknown messages service.");

It seems like not all callers will handle this exception gracefully?

::: services/sync/modules/engines/clients.js:899
(Diff revision 3)
>     * topic. The callback will receive an array as the subject parameter
>     * containing objects with the following keys:
>     *
> -   *   uri       URI (string) that is requested for display.
> -   *   clientId  ID of client that sent the command.
> -   *   title     Title of page that loaded URI (likely) corresponds to.
> +   *   uri        URI (string) that is requested for display.
> +   *   clientId   ID of client that sent the command.
> +   *   senderName Name of client that sent the command.

Can you remind me again why we are sending the name around rather than the fxa id?
Attachment #8957360 - Flags: review?(markh)
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

Thom, do you mind having a look at this? Feel free to disagree with any of my points (I'm sure Ed would be happy if you did ;)
Attachment #8957360 - Flags: feedback?(tchiovoloni)
Priority: P2 → P1
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review236806

::: browser/base/content/browser-sync.js:332
(Diff revision 3)
> +    for (const client of clients) {
> +      const device = devices.find(d => d.id == client.fxaDeviceId);
> +      if (device && fxAccounts.isMessagesAwareDevice(device)) {
> +        toSendMessages.push(device);
> +      } else {
> +        Weave.Service.clientsEngine.sendURIToClientForDisplay(url, client.id, title).catch(e => {

Do we want to await these also?

::: services/fxaccounts/FxAccountsClient.jsm:480
(Diff revision 3)
> +   * @param  sessionTokenHex - Session token obtained from signIn
> +   * @param  service
> +   * @param  to - Recipient device ID.
> +   * @param  data
> +   * @return Promise
> +   *         Resolves to an empty object:

nit: Probably say something like "Resolves to the request's response, (which should be an empty object)" or something along those lines.

::: services/fxaccounts/FxAccountsPush.js:253
(Diff revision 3)
>    },
> +
> +  /**
> +   * Get our Push server subscription.
> +   *
> +   * Ref: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPushService#getSubscription()

Here's hoping this URL doesn't bitrot...

::: services/fxaccounts/FxAccountsPush.js:263
(Diff revision 3)
> +    return new Promise((resolve) => {
> +      this.pushService.getSubscription(FXA_PUSH_SCOPE_ACCOUNT_UPDATE,
> +        Services.scriptSecurityManager.getSystemPrincipal(),
> +        (result, subscription) => {
> +          if (!subscription) {
> +            return resolve(null);

Should we log here? Or even reject?
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review238566

Whoops, hit the wrong button. I don't have much to say other than that this looks fine to me, modulo nits.
Attachment #8957360 - Flags: review+
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

Actually, f+ is more accurate since it's what was requested, and I don't have 100% of the context. Sorry for all the updates.
Attachment #8957360 - Flags: review+
Attachment #8957360 - Flags: feedback?(tchiovoloni)
Attachment #8957360 - Flags: feedback+
Amended (thanks for the comments). It's a sizeable amend (since some things changed on the server side).
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review240818

Nice work Ed!

::: browser/base/content/browser-sync.js:323
(Diff revision 4)
>      url += "send-tabs/?utm_source=" + Services.appinfo.name.toLowerCase();
>      switchToTabHavingURI(url, true, { replaceQueryString: true });
>    },
>  
> -  sendTabToDevice(url, clientId, title) {
> -    Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title).catch(e => {
> +  async sendTabToDevice(url, clients, title) {
> +    const devices = await fxAccounts.getDeviceList();

I think we should consider wrapping this in a try/catch and a console.error

::: browser/components/nsBrowserGlue.js:2567
(Diff revision 4)
>          if (wasTruncated) {
>            body = bundle.formatStringFromName("singleTabArrivingWithTruncatedURL.body", [body], 1);
>          }
>        } else {
>          title = bundle.GetStringFromName("multipleTabsArrivingNotification.title");
> -        const allSameDevice = URIs.every(URI => URI.clientId == URIs[0].clientId);
> +        const allSameDevice = URIs.every(URI => URI.senderName == URIs[0].senderName);

In comment 16 I asked about why we are sending the name around rather than the ID. What worries me here is that a user who uses multiple profiles on the same PC is very likely to end up with them having the same name by default, which seems like a future bug waiting to be opened. Is there something we can do about this in a followup?

::: services/fxaccounts/FxAccounts.jsm:438
(Diff revision 4)
>      let storage = new FxAccountsStorageManager();
>      storage.initialize(credentials);
>      return new AccountState(storage);
>    },
>  
> +  // "Friend" classes of FxAccounts (e.g. FxAccountsMessages) know about the

nice!

(Although maybe passing the functions in an object is more future-proof? ie, `async ({getUserData, updateUserData})` or similar - but I don't really care.

::: services/fxaccounts/FxAccountsClient.jsm:455
(Diff revision 4)
>    /**
> +   * Retrieves messages from our device's message box.
> +   *
> +   * @method getMessages
> +   * @param  sessionTokenHex - Session token obtained from signIn
> +   * @param  [index]

We really should have a brief description of the params here.

::: services/fxaccounts/FxAccountsMessages.js:5
(Diff revision 4)
> +/* 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/. */
> +
> +const EXPORTED_SYMBOLS = ["FxAccountsMessages", /* the rest is for testing only */

nit - s/is/are/

::: services/fxaccounts/FxAccountsMessages.js:50
(Diff revision 4)
> +    const ourDeviceId = await this.fxAccounts.getDeviceId();
> +    const payload = {
> +      topic: TOPICS.SEND_TAB,
> +      data: {
> +        from: ourDeviceId,
> +        entries: [{title: tab.title, url: tab.url}]

IIUC, this could be written as `[(title, url}=tab)]` (which isn't really an improvement, just thought I'd mention it in case you think it is :)

::: services/fxaccounts/FxAccountsMessages.js:119
(Diff revision 4)
> +      try {
> +        const bytes = await this._decrypt(data, keys);
> +        const payload = JSON.parse(decoder.decode(bytes));
> +        payloads.push(payload);
> +      } catch (e) {
> +        log.error(`Could not unwrap message ${index}`);

`e` should be logged here.

::: services/fxaccounts/FxAccountsMessages.js:128
(Diff revision 4)
> +      await this.handler.handle(payloads);
> +    }
> +  }
> +
> +  async _fetchMessages() {
> +    log.info(`Fetching unread messages.`);

To prevent log noise I think you should wither remove the log at the top of consumeRemoteMessages or change it to .debug, and move this log down so that you can also log the index we are using - it might help us diagnose repeated messages or similar.

::: services/sync/modules/policies.js:520
(Diff revision 4)
>      if (!Async.isAppReady()) {
>        this._log.debug("Not initiating sync: app is shutting down");
>        return;
>      }
>      Services.tm.dispatchToMainThread(() => {
> +      // Terrible hack bellow: we do the fxa messages polling in the sync

s/bellow/below/ - and I guess we want a bug for the "XXX" (or if you don't think it's worth opening one, just remove the XXX comment)

::: services/sync/modules/policies.js:523
(Diff revision 4)
>      }
>      Services.tm.dispatchToMainThread(() => {
> +      // Terrible hack bellow: we do the fxa messages polling in the sync
> +      // scheduler to get free post-wake/link-state etc detection.
> +      // See bug XXX.
> +      fxAccounts.messages.consumeRemoteMessages();

I assume you intend for the fetch to run at the same time as the sync (which seems fine to me TBH)? If so, we should have a trailing .catch here
Attachment #8957360 - Flags: review?(markh) → review+
Comment on attachment 8957360 [details]
Bug 1442133 - FxA messages client implementation.

https://reviewboard.mozilla.org/r/226292/#review241160

::: services/fxaccounts/FxAccounts.jsm:1722
(Diff revision 5)
>          log.debug("registering new device details");
>          device = await this.fxAccountsClient.registerDevice(
>            sessionToken, deviceName, this._getDeviceType(), deviceOptions);
>        }
>  
>        await this.currentAccountState.updateUserAccountData({

Note to myself: This will overwrite persisted message indexes on registration update.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ef95c56df04
FxA messages client implementation. r=markh,tcsc
https://hg.mozilla.org/mozilla-central/rev/8ef95c56df04
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.