Enable Sync to communicate with Firefox Accounts via WebChannels.

VERIFIED FIXED in Firefox 40

Status

()

defect
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: stomlinson, Assigned: stomlinson)

Tracking

38 Branch
Firefox 40
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox38 unaffected, firefox38.0.5 unaffected, firefox39 affected, firefox40 verified)

Details

(URL)

Attachments

(1 attachment, 12 obsolete attachments)

42.74 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Using the WebChannel abstraction will allow Firefox Accounts to communicate with Sync when FxA is embedded in an IFRAME.
(Assignee)

Updated

4 years ago
Blocks: 1142046, 1146688
(Assignee)

Updated

4 years ago
Depends on: CVE-2015-2718
(Assignee)

Updated

4 years ago
Assignee: nobody → stomlinson
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Attachment #8582408 - Flags: feedback?(mhammond)
Attachment #8582408 - Flags: feedback?(ckarlof)
(Assignee)

Comment 2

4 years ago
I created the first patch prematurely. This replaces the first.
Attachment #8582408 - Attachment is obsolete: true
Attachment #8582408 - Flags: feedback?(mhammond)
Attachment #8582408 - Flags: feedback?(ckarlof)
Attachment #8582412 - Flags: feedback?(mhammond)
(Assignee)

Comment 3

4 years ago
:markh - This is my first hg/bugzilla based patch, I am still trying to figure out the workflow. Obviously there are a couple of files with whitespace only changes that need to be removed from the patch. I updated my .vimrc to no longer remove EOL whitespace. 

I am unsure of m-c best practices regarding the use of mock objects for unit tests. I would like to pass in an FxAccountsSyncCommon object to avoid a actual calls to login and shouldAllowRelink.

The FxAccountsWebChannel is created in browser/base/content/browser-fxaccounts.js. gFxAccounts is initialized in browser.js->delayedStartup. Your email suggestion was to create the FxAccountsWebChannel in browser.js->delayedStartup, so this seemed appropriate.
(Assignee)

Comment 4

4 years ago
This patch adds more FxAccountsWebChannel unit tests and removes whitespace only changes.
Attachment #8582412 - Attachment is obsolete: true
Attachment #8582412 - Flags: feedback?(mhammond)
Attachment #8583109 - Flags: feedback?(mhammond)
Attachment #8583109 - Flags: feedback?(ckarlof)
Comment on attachment 8583109 [details] [diff] [review]
bug-1146904-sync-via-webchannel-003.patch

Review of attachment 8583109 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: browser/base/content/browser-fxaccounts.js
@@ +90,5 @@
>  
> +    // The FxAccountsWebChannel listens for events and updates
> +    // the state machine accordingly.
> +    let fxAccountsWebChannel = new FxAccountsWebChannel({
> +      content_uri: 'http://127.0.0.1:3030',

We might as well add the pref name we will use now

@@ +91,5 @@
> +    // The FxAccountsWebChannel listens for events and updates
> +    // the state machine accordingly.
> +    let fxAccountsWebChannel = new FxAccountsWebChannel({
> +      content_uri: 'http://127.0.0.1:3030',
> +      channel_id: 'sync'

let's call the channel something about FxA (and it looks like you should be using SYNC_WEBCHANNEL_ID?)

::: services/fxaccounts/FxAccountsSyncCommon.jsm
@@ +1,2 @@
> +/* 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

I'm still not convinced we need this module - IMO it should be in FxAccountsWebChannel
Attachment #8583109 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 6

4 years ago
Thanks for the feedback Mark. I have some questions about approach and testing.

When creating the FxAccountsWebChannel, a channel_id is required. This is different to FxAccountsProfileWebChannel which hard codes its channel id to the FxAccountsCommon.PROFILE_WEBCHANNEL_ID. I require a channel_id because IIUC, you would like to see the SyncWebChannel and the ProfileWebChannel combined. I thought this would be an easy way to combine the code but still allow instances to be created with either the profile or sync channel name. Is this reasonable? Do you still think both WebChannels should be combined? If so, Zach and I will have to coordinate on merging the two. Perhaps as a follow on PR once both of our respective pieces are stable?

You mentioned you the code in FxAccountsSyncCommon.jsm should be rolled into FxAccountsWebChannel.jsm. All of that code is an extraction from browser/base/content/aboutaccounts/aboutaccounts.js, minus the about:accounts specific UI code. My logic was that short of converting about:accounts to be FxAccountsWebChannel driven, aboutaccounts.js could use the code from FxAccountsSyncCommon.js and get rid of its local copy. If aboutaccounts.js will be converted to be FxAccountsWebChannel driven, then yeah, FxAccountsSyncCommon.jsm is unnecessary.

Does the code aboutaccounts.js have unit tests? If so, I could use them as a base to work from. It looks like there are functional tests in /browser/base/content/test/general/browser_aboutAccounts.js.
Flags: needinfo?(mhammond)
(In reply to Shane Tomlinson [:stomlinson] from comment #6)

> When creating the FxAccountsWebChannel, a channel_id is required. This is
> different to FxAccountsProfileWebChannel which hard codes its channel id to
> the FxAccountsCommon.PROFILE_WEBCHANNEL_ID. I require a channel_id because
> IIUC, you would like to see the SyncWebChannel and the ProfileWebChannel
> combined. I thought this would be an easy way to combine the code but still
> allow instances to be created with either the profile or sync channel name.
> Is this reasonable? Do you still think both WebChannels should be combined?

Yeah, I don't see a good reason to have separate channels - I think the 2 are logically related - so IMO the same channel makes sense.  This would involve renaming FxAccountsProfileChannel.jsm to something lie FxAccountsChannel.jsm or similar.

So IIUC, you'd use the same "channel" ID but different "command" IDs.

> If so, Zach and I will have to coordinate on merging the two. Perhaps as a
> follow on PR once both of our respective pieces are stable?

Zach's has landed now, so you should just be able to tweak his.

> You mentioned you the code in FxAccountsSyncCommon.jsm should be rolled into
> FxAccountsWebChannel.jsm. All of that code is an extraction from
> browser/base/content/aboutaccounts/aboutaccounts.js, minus the
> about:accounts specific UI code. My logic was that short of converting
> about:accounts to be FxAccountsWebChannel driven, aboutaccounts.js could use
> the code from FxAccountsSyncCommon.js and get rid of its local copy. If
> aboutaccounts.js will be converted to be FxAccountsWebChannel driven, then
> yeah, FxAccountsSyncCommon.jsm is unnecessary.

I think we want to get rid of about:accounts sooner rather than later, so a common module seems unnecessary, but I'm not too bothered by that TBH - "less is more" is my current motto ;)

> Does the code aboutaccounts.js have unit tests? If so, I could use them as a
> base to work from. It looks like there are functional tests in
> /browser/base/content/test/general/browser_aboutAccounts.js.

Only those functional requests.

Zaaaaaaach, do you have any thoughts on the above?
Flags: needinfo?(mhammond) → needinfo?(zack.carter)
I chatted with Shane today and we went over some of the questions he had. The above sounds about right.
Flags: needinfo?(zack.carter)

Updated

4 years ago
Blocks: 1150222
Will this land in 38.x or likely in 39.0 given the blocker?
(In reply to Chris More [:cmore] from comment #9)
> Will this land in 38.x or likely in 39.0 given the blocker?

Realistically, we're looking at 39 now.
Shane, I know I said to take the f? off for me, but I'd like to think a bit about the code organization here. If you have time tomorrow (your Thursday) evening to chat about this, I'd love to grab you for a bit.
(In reply to Chris Karlof [:ckarlof] from comment #10)
> (In reply to Chris More [:cmore] from comment #9)
> > Will this land in 38.x or likely in 39.0 given the blocker?
> 
> Realistically, we're looking at 39 now.

39 is for us too as we are locked in for for 38.1 with the fall back variation and button.
(Assignee)

Comment 13

4 years ago
This patch renames the FxAccountsProfileChannel to FxAccountsWebChannel and adds Sync functionality. There are still two web channels created, this will need to be updated. The origin FxAccountsProfileChannel is created in services/fxaccounts/FxAccountsProfile.jsm, I create a new channel browser/base/content/browser-fxaccounts.js which creates the channel on browser startup.
Attachment #8583109 - Attachment is obsolete: true
Attachment #8583109 - Flags: feedback?(ckarlof)
Attachment #8588593 - Flags: feedback?(zack.carter)
Attachment #8588593 - Flags: feedback?(mhammond)
Comment on attachment 8588593 [details] [diff] [review]
Version 4 - Rename and update Zach's FxAccountsProfileChannel to FxAccountsWebChannel, add Sync related functionality.

Review of attachment 8588593 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, and IIUC your most recent comment, I agree it makes sense to only create a single channel via browser.js and kill the one created by the fxa profile code.

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +7,1 @@
>   * Uses the WebChannel component to receive messages about account changes.

It seems this comment should be updated slightly - just "... about account state changes" is probably enough (but whatever you think - it's not currently *wrong* :)

@@ +7,4 @@
>   * Uses the WebChannel component to receive messages about account changes.
>   */
>  
> +this.EXPORTED_SYMBOLS = ["FxAccountsWebChannel", "FxAccountsWebChannelHelpers"];

Do we need to export the helpers?  If just for tests, you might be surprised to learn that the "backstage pass" uses by tests will already export this for you (I was when I discovered that!)

@@ +121,5 @@
>       * @private
>       */
>      let listener = (webChannelId, message, target) => {
>        if (message) {
> +        log.debug("WebChannel message received: " + JSON.stringify(message));

note you can just write:

      log.debug("WebChannel message received", message);

to get the same output, with the (theoretical) advantage that that we don't pay for the stringify if the logs levels are set such that the message isn't written anywhere (and I say "theoretical" as in this context I think the logs are .Debug by default, so in this case there's no practical advantage)

@@ +126,5 @@
>          let command = message.command;
>          let data = message.data;
> +
> +        let messageId = message.messageId;
> +        let response = new FxAccountsWebChannelResponse(this._channel, target, command, messageId);

it looks like the response object (and thus messageId) should be created in the CAN_LINK_ACCOUNT case?

@@ +164,5 @@
> +  // a modal dialog checking they really really want to do this...
> +  // (This is sync-specific, so ideally would be in sync's identity module,
> +  // but it's a little more seamless to do here, and sync is currently the
> +  // only fxa consumer, so...
> +  shouldAllowRelink: function shouldAllowRelink(acctName) {

I think we should start using the new streamlined syntax - ie:
  shouldAllowRelink(acctName) {
    ...
  },

@@ +174,5 @@
> +    Services.prefs.setBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION, showCustomizeSyncPref);
> +  },
> +
> +  getShowCustomizeSyncPref: function (showCustomizeSyncPref) {
> +    return Services.prefs.getBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION);

not sure this helper makes much sense (ie, just using Services.prefs.getBoolPref inline seems fine to me.

@@ +221,5 @@
> +    Services.prefs.setComplexValue(PREF_LAST_FXA_USER, Ci.nsISupportsString, string);
> +  },
> +
> +  // Given a string, returns the SHA265 hash in base64
> +  sha256: function sha256(str) {

I wonder if this should just be a module-level helper given it doesn't reference 'this'

@@ +276,5 @@
> +    messageId: this._messageId,
> +    data: data
> +  };
> +
> +  log.debug("WebChannel response: " + JSON.stringify(data));

ditto here we the .stringify

::: services/fxaccounts/tests/xpcshell/test_web_channel.js
@@ +18,3 @@
>    "Error: Missing 'content_uri' option");
>  
> +  validationHelper({ 

trailing whitespace
Attachment #8588593 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 15

4 years ago
Comment on attachment 8588593 [details] [diff] [review]
Version 4 - Rename and update Zach's FxAccountsProfileChannel to FxAccountsWebChannel, add Sync related functionality.

Review of attachment 8588593 [details] [diff] [review]:
-----------------------------------------------------------------

Mark, I made some inline comments and have created another patch based on your feedback, but I'm a bit hesitant to issue it before Zach has had a chance to give his input too. I don't want to overload you with too many reviews since you have other work going on.

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +7,4 @@
>   * Uses the WebChannel component to receive messages about account changes.
>   */
>  
> +this.EXPORTED_SYMBOLS = ["FxAccountsWebChannel", "FxAccountsWebChannelHelpers"];

Thanks Mark, I did not know that, but have found out how.

@@ +121,5 @@
>       * @private
>       */
>      let listener = (webChannelId, message, target) => {
>        if (message) {
> +        log.debug("WebChannel message received: " + JSON.stringify(message));

done.

@@ +126,5 @@
>          let command = message.command;
>          let data = message.data;
> +
> +        let messageId = message.messageId;
> +        let response = new FxAccountsWebChannelResponse(this._channel, target, command, messageId);

done.

@@ +164,5 @@
> +  // a modal dialog checking they really really want to do this...
> +  // (This is sync-specific, so ideally would be in sync's identity module,
> +  // but it's a little more seamless to do here, and sync is currently the
> +  // only fxa consumer, so...
> +  shouldAllowRelink: function shouldAllowRelink(acctName) {

So much new goodness that I haven't been able to use in normal web dev land. Updated.

@@ +174,5 @@
> +    Services.prefs.setBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION, showCustomizeSyncPref);
> +  },
> +
> +  getShowCustomizeSyncPref: function (showCustomizeSyncPref) {
> +    return Services.prefs.getBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION);

I did it this way because I call the function in the unit tests to see if the value was set. I suppose the unit tests could actually check the preferences, though we would end up repeating this line in several places.

@@ +221,5 @@
> +    Services.prefs.setComplexValue(PREF_LAST_FXA_USER, Ci.nsISupportsString, string);
> +  },
> +
> +  // Given a string, returns the SHA265 hash in base64
> +  sha256: function sha256(str) {

Again, I use this in the unit tests. I can avoid using this function in the tests by ensuring the final hash is not the same as the previous hash, which may be sufficient.

@@ +276,5 @@
> +    messageId: this._messageId,
> +    data: data
> +  };
> +
> +  log.debug("WebChannel response: " + JSON.stringify(data));

done.
(In reply to Shane Tomlinson [:stomlinson] from comment #15)

> I did it this way because I call the function in the unit tests to see if
> the value was set.

Tests are a good justification :)  Maybe a short comment above such functions with a comment like "A hook-point for tests"?
Comment on attachment 8588593 [details] [diff] [review]
Version 4 - Rename and update Zach's FxAccountsProfileChannel to FxAccountsWebChannel, add Sync related functionality.

Review of attachment 8588593 [details] [diff] [review]:
-----------------------------------------------------------------

I've tested it and found no regressions w.r.t. profile images. Once this lands we can switch the content server over to using the "fx_account" channel for profile changes, then we can remove references to that channel from the browser.
Attachment #8588593 - Flags: feedback?(zack.carter) → feedback+
(Assignee)

Comment 18

4 years ago
This revision makes the updates requested by markh:
* Use "new" more terse syntax for object member functions.
* Only a single FxAccountsWebChannel is created in browser/base/content/browser-fxaccounts.js
* mochitests renamed from browser_fxa_profile_channel.* to browser_fxa_web_channel.* with additional tests added for 'fxaccounts:can_link_account', and 'fxaccounts:login'
* The FxAccountsWebChannelResponse object is only created for the 'fxaccounts:can_link_account' message.
* FxAccountsWebChannelHelpers is no longer exposed by FxAccountsWebChannel.jsm.
Attachment #8588593 - Attachment is obsolete: true
Attachment #8589671 - Flags: review?(zack.carter)
Attachment #8589671 - Flags: review?(mhammond)
Comment on attachment 8589671 [details] [diff] [review]
Version 5 - Rename and update Zach's FxAccountsProfileChannel to FxAccountsWebChannel, add Sync related functionality.

Review of attachment 8589671 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: browser/base/content/test/general/browser_fxa_web_channel.html
@@ +63,5 @@
> +
> +  function test_can_link_account() {
> +    window.addEventListener("WebChannelMessageToContent", function (e) {
> +      // echo any responses from the browser back to the tests on the
> +      // fxaccounts_webchannel_response_echo WebChannel. The tests are 

nit: trailing whitespace

::: browser/base/content/test/general/browser_fxa_web_channel.js
@@ +23,1 @@
>      run: function* () {

none of these test function should be generators (ie, should not have the '*') as they don't yield - I know you copied them, but we might as well remove them all.

@@ +23,4 @@
>      run: function* () {
>        return new Promise(function(resolve, reject) {
>          let tabOpened = false;
> +        let properUrl = "http://example.com/browser/browser/base/content/test/general/browser_fxa_web_channel.html?profile_change";

can we put the base part of the URL in a const at the top of the file?, ie:
const TEST_HTML = "http://...";
...
let properUrl + TEST_HTML + "?profile_change"; (and + "?login" etc)

@@ +97,5 @@
> +        let tabOpened = false;
> +        let properUrl = "http://example.com/browser/browser/base/content/test/general/browser_fxa_web_channel.html?can_link_account";
> +
> +        let webChannelOrigin = Services.io.newURI(properUrl, null, null);
> +        // responses sent to content are echoed back over the 

couple more trailing spaces

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +4,5 @@
>  
>  /**
> + * Firefox Accounts Web Channel.
> + *
> + * Uses the WebChannel component to receive messages 

trailing space

@@ +55,5 @@
> +  }
> +  this._webChannelId = options.channel_id;
> +
> +  let helpers = new FxAccountsWebChannelHelpers(options);
> +  this._helpers = options.helpers || helpers;

I think it's worth a comment here that options.helpers is specified only by tests.

@@ +123,5 @@
>       * @private
>       */
>      let listener = (webChannelId, message, target) => {
>        if (message) {
> +        log.debug("WebChannel message received: ", message);

note that the trailing ": " is not necessary - it will be appended by the log.

@@ +144,1 @@
>          }

I think a default handler that logs an warning (error?) about an unrecognized command might make sense.

@@ +144,5 @@
>          }
>        }
>      };
>  
>      this._channelCallback = listener;

I know this already existed, but best I can tell, this._channelCallback isn't used. Unless I'm missing something, please remove it.

@@ +253,5 @@
> +    return hasher.finish(true);
> +  },
> +
> +  /**
> +   * If a user signs in using a different account, the data from the 

trailing space

@@ +292,5 @@
> +    return pressed === 0; // 0 is the "continue" button
> +  }
> +};
> +
> +function FxAccountsWebChannelResponse(channel, target, message) {

I meant to mention this before, but I'm not sure this object adds much value given it is only used once, and would only take 7 lines if inline vs the ~20 it takes here.  I'm not too bothered by this, so consider it a suggestion :)

@@ +307,5 @@
> +      messageId: this._messageId,
> +      data: data
> +    };
> +
> +    log.debug("WebChannel response: ", data);

ditto here re trailing ": "
Attachment #8589671 - Flags: review?(mhammond) → review+
(Assignee)

Comment 20

4 years ago
Comment on attachment 8589671 [details] [diff] [review]
Version 5 - Rename and update Zach's FxAccountsProfileChannel to FxAccountsWebChannel, add Sync related functionality.

Review of attachment 8589671 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +55,5 @@
> +  }
> +  this._webChannelId = options.channel_id;
> +
> +  let helpers = new FxAccountsWebChannelHelpers(options);
> +  this._helpers = options.helpers || helpers;

Comment added in the param docs above.

@@ +123,5 @@
>       * @private
>       */
>      let listener = (webChannelId, message, target) => {
>        if (message) {
> +        log.debug("WebChannel message received: ", message);

`: ` removed.

@@ +144,1 @@
>          }

Added, and a test to ensure unexpected commands do not cause an error has been added too.

@@ +144,5 @@
>          }
>        }
>      };
>  
>      this._channelCallback = listener;

This is used by the unit tests to get access to the listener method directly.

@@ +292,5 @@
> +    return pressed === 0; // 0 is the "continue" button
> +  }
> +};
> +
> +function FxAccountsWebChannelResponse(channel, target, message) {

Removed.
(Assignee)

Comment 21

4 years ago
This patch addresses markh's comments from Version 5.

* Trailing whitespace
* Remove the FxAccountsWebChannelResponse abstraction.
* Document when the "helpers" parameter is specified.
* console log cleanup
* Add a `default` command handler to the web channel listener function.
Attachment #8589671 - Attachment is obsolete: true
Attachment #8589671 - Flags: review?(zack.carter)
Attachment #8590258 - Flags: review?(mhammond)
(Assignee)

Comment 22

4 years ago
Mark, I imagine you need to do a re-review just to make sure everything is in order. Once this is approved, what are the next steps? I am looking at the MDN guide to having someone with commit access to commit the patch [1], as far as I can tell the patch meets the requirements.

For this to work smoothly with the new first run flow, we depend on the fix for #1146724.

Thanks for all the help!

--------------------------


[1] - https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(Assignee)

Comment 23

4 years ago
The patch for the content server to update the channel name: https://github.com/mozilla/fxa-content-server/pull/2312. To avoid problems with testing/development, the content server patch should be merged after this lands in Nightly.
(Assignee)

Comment 24

4 years ago
:zaach has pointed out that changing the WebChannel ID from `account_updates` to `fx_accounts` will break the profile integration with the current production FxA code. A user could edit their about:config to point to `latest`, but this is not the ideal way to test his new feature.

The content server could work around the problem by temporarily sending messages on two WebChannels, one with each ID. This patch would be blocked until the content server change makes its way to prod, which is less than ideal if we want this to be uplifted to 39.

Mark, would you be against reverting the WebChannel ID? If not, I'll update the patch.
Flags: needinfo?(mhammond)
We could leave it as `account_updates` for now, then switch it to `fx_accounts` when the content-server changes hit production.
(Assignee)

Comment 26

4 years ago
To expand on Zach's proposal:

1. Temporarily leave the WebChannel ID in Firefox as `account_updates`. 
2. Issue a PR to the content server to send messages on two WebChannels with the IDs `account_updates` & `fx_accounts`. 
3. When the content server change makes its way to prod, issue a new Firefox patch to change the WebChannel ID to `fx_accounts`. 
4. When the 2nd Fx patch lands, issue a PR to the content server to send to only one channel - `fx_accounts` This PR is cleanup and not part of the critical path.

Upsides of this approach:
* The channel ID eventually becomes sensible.
* Zach's profile/avatar work to continue to function without edits to about:config

Downside:
* The WebChannel ID is changed.
* A lot of patches. Fx 39 is already in Aurora, this approach requires landing and uplifting 2 patches to Firefox, and at least one to the content server.
* The Sync portion of the content server code will become more complex - we'll have to send/listen for responses on two channels until step 4 is complete.

Leaving the WebChannel ID `account_updates` forever and avoiding the thrash sounds appealing. A `login` or `can_link_account` message could be considered an account update, no?
Comment on attachment 8590258 [details] [diff] [review]
bug-1146904-sync-via-webchannel-006.patch

LGTM, and generally there's no need to re-request review if you have only addressed comments in a previous r+.  To get it checked in, just set the "checkin-needed" keyword.

That still leaves us with the question of bug 1146724 - it's not clear this should land until that does.  You may like to take that bug - IIUC Chris's comments correctly, it should just be a matter of passing an eventOrigin string along with the new "eventSender" and checking the eventSender still has that origin when it comes to dispatching it (and as per MattN's comment, make sure we don't break unsolicited chrome->content messages.)  Feel free to take my patch and make it your own :)

Re the channel name - I'm fine with reverting the channel name for the reasons you indicate.
Flags: needinfo?(mhammond)
Attachment #8590258 - Flags: review?(mhammond) → review+
And for a million bonus points, consider opening a new bug to kill about:accounts ;)
(In reply to Shane Tomlinson [:stomlinson] from comment #26)
> 
> Leaving the WebChannel ID `account_updates` forever and avoiding the thrash
> sounds appealing. A `login` or `can_link_account` message could be
> considered an account update, no?

SGTM
(Assignee)

Comment 30

4 years ago
This patch reverts the FirefoxAccountsWebChannel ID to `account_updates`.

Thanks Mark and Zach for the feedback, help and reviews!
Attachment #8590258 - Attachment is obsolete: true
Attachment #8591270 - Flags: checkin?(mhammond)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 31

4 years ago
> And for a million bonus points, consider opening a new bug to kill about:accounts ;)

You'll be happy to know that Ryan Feeley and John Gruen already have designs for this. I've seen them. They are nice :D

See https://bugzilla.mozilla.org/show_bug.cgi?id=1152385
(Assignee)

Updated

4 years ago
Blocks: 1152385
Comment on attachment 8591270 [details] [diff] [review]
bug-1146904-sync-via-webchannel-007.patch

Never seen this flag before, so no idea if it means "ok to checkin" or "has been checked in" :)
Attachment #8591270 - Flags: checkin?(mhammond)
(Assignee)

Comment 33

4 years ago
> Never seen this flag before, so no idea if it means "ok to checkin" or "has been checked in" :)

OK, I won't set that next time. Checkin needed. :D
(Assignee)

Comment 34

4 years ago
:markh - I just found a bug in `_promptForRelink`, the button check is missing trailing `+`. I think I accidentally removed them when running an invalid regexp to remove trailing whitespace.
(Assignee)

Comment 35

4 years ago
Diff vs 007 - This patch re-adds 3 missing end of line `+`s in _promptForRelink
Attachment #8591270 - Attachment is obsolete: true
Comment on attachment 8591642 [details] [diff] [review]
bug-1146904-sync-via-webchannel-008.patch

Review of attachment 8591642 [details] [diff] [review]:
-----------------------------------------------------------------

Profile images still looking good.
Attachment #8591642 - Flags: feedback+
Shane: what the ETA on when this bug will be resolved and verified? How much risk is involved with uplifting to 38?
Flags: needinfo?(stomlinson)
(Assignee)

Comment 38

4 years ago
> Shane: what the ETA on when this bug will be resolved and verified? How much risk is involved with uplifting to 38?

This is blocked on finding a reviewer for #1146724. Without that fix, the chances of this patch landing are very small. Follow that bug for updates.

No chance of 38, we were always targeting 39. Mark understands the importance and has asked Gavin if he could review/nominate alternative reviewers.
Flags: needinfo?(stomlinson)
:gavin: Did you find alternative reviewers for this?
Flags: needinfo?(gavin.sharp)
MattN is helping with the reviews.
Flags: needinfo?(gavin.sharp)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1046535
(Assignee)

Comment 42

4 years ago
Diff over patch 008:

* Tests updated to use BrowserTestUtils.withNewTab instead of the local waitForTab.
* Ensure all tests pass with the final patch for 1146724.
* in FxAccountsWebChannel.jsm->listener, change `target` to `sendingContext` and update the documentation.
Attachment #8591642 - Attachment is obsolete: true
Attachment #8597693 - Flags: review?(zack.carter)
Attachment #8597693 - Flags: review?(mhammond)
(Assignee)

Comment 43

4 years ago
:markh and :zaach,
I have created one final patch to ensure the tests pass with the final patch for #1146724. Functionality is identical, this primarily deals tests and ensuring consistency with the referenced WebChannel patch.
Comment on attachment 8597693 [details] [diff] [review]
bug-1146904-sync-via-webchannel-009.patch

Review of attachment 8597693 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +143,2 @@
>              Services.obs.notifyObservers(null, ON_PROFILE_CHANGE_NOTIFICATION, data.uid);
>            break;

The "break" is typically aligned with the block itself (not sure why the existing one slipped past)
Attachment #8597693 - Flags: review?(mhammond) → review+
(Assignee)

Comment 45

4 years ago
Diff from 009:

* `break` indentation.
Attachment #8597693 - Attachment is obsolete: true
Attachment #8597693 - Flags: review?(zack.carter)
Attachment #8597999 - Flags: review?(zack.carter)
Comment on attachment 8597999 [details] [diff] [review]
bug-1146904-sync-via-webchannel-010.patch

Review of attachment 8597999 [details] [diff] [review]:
-----------------------------------------------------------------

No regressions with avatars found.

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +173,5 @@
>  
> +this.FxAccountsWebChannelHelpers = function(options) {
> +  options = options || {};
> +
> +  this._fxAccounts = options.fx_accounts || fxAccounts;

Nit: may want to use camelCase for consistency.
Attachment #8597999 - Flags: review?(zack.carter) → review+
(Assignee)

Comment 47

4 years ago
Comment on attachment 8597999 [details] [diff] [review]
bug-1146904-sync-via-webchannel-010.patch

Review of attachment 8597999 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +173,5 @@
>  
> +this.FxAccountsWebChannelHelpers = function(options) {
> +  options = options || {};
> +
> +  this._fxAccounts = options.fx_accounts || fxAccounts;

Thanks :zaach, I used snake_case because that seems to be the convention for input options elsewhere. I'm not sure which is more clear.
(Assignee)

Comment 48

4 years ago
Diff from 010:

* replace the `fx_accounts` input parameter to FxAccountsWebChannelHelpers with `fxAccounts` for consistency with the non-mocked object.
Attachment #8597999 - Attachment is obsolete: true
(Assignee)

Comment 49

4 years ago
Thanks :zaach and :markh for the continued help, I have just added the "checkin-needed" keyword.
Keywords: checkin-needed
Please provide a green Try link for this patch :)
Keywords: checkin-needed
(Assignee)

Comment 51

4 years ago
Hi :RyanVM, I do not have push access to the Try server either. I have a tab to bugzilla with a bug asking for access ready to submit, but need a voucher.
Posted patch 0002-fix-tests.patch (obsolete) — Splinter Review
The tests failed on try.  test_profile.js broke due to ._listenForProfileChanges being removed. Now that the channel is created as the browser starts I don't think these parts are needed, so I just removed them. Zaach, do the changes to test_profile.js look ok?

browser_fxa_web_channel.js failed on e10s due to timing issues (or more accurately, it passed on non-e10s due to timing issues ;) 2 things seemed to conspire here - the same channel ID was used as the real channel, which caused strangeness, and the way the test was structured, the test html had already sent its channel message before BrowserTestUtils.withNewTab() returned. The fix here is to change the channel ID, and also to setup the listeners etc *before* calling BrowserTestUtils.withNewTab() but still have it wait in the promise.

Shane, if you can have a look over these changes, and assuming Zaach is also happy (and that the try is green), re-upload the patch with these changes rolled in an re-request checkin-needed - no need to re-request review nor to fire another test run if these changes are rolled up.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5160066a9257
Attachment #8600761 - Flags: feedback?(zack.carter)
This patch is relevant to two efforts on Android:

* seamlessly logging into Firefox Accounts-backed web properties (Bug 1158869);
* exposing an OAuth flow on Android to third-party Apps (Bug 1158875);
* and even abandoning our native sign-in flow and using the web flow.

So happy to see progress here!
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 54

4 years ago
:markh, the test changes look good to me, and understandable. Thanks for figuring out the e10s problems, that would have taken me all week.

:zaach, what do you think?
Flags: needinfo?(zack.carter)
Comment on attachment 8600761 [details] [diff] [review]
0002-fix-tests.patch

Review of attachment 8600761 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8600761 - Flags: feedback?(zack.carter) → feedback+
Flags: needinfo?(zack.carter)
(Assignee)

Comment 56

4 years ago
Diff from 011:
* Contains markh's patch from https://bugzilla.mozilla.org/attachment.cgi?id=8600761 that fixes the profile and e10s tests.
Attachment #8599864 - Attachment is obsolete: true
Attachment #8600761 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Bugger - we missed that error in the try push. Shane, if we don't need this channel on b2g, the simple fix would be to skip it on b2g in xpcshell.ini.  However, I'm not completely sure we *can* just skip the channel tests there (eg, the profile stuff might be needed) in which case we probably need to arrange to gracefully avoid the Weave service stuff in the .jsm itself.
(Assignee)

Comment 63

4 years ago
:KWierso markh informs me that he updated the patch to fix the failing test, the new patch has been pushed to fx-team. 

https://hg.mozilla.org/integration/fx-team/rev/eac6ac60b5e6
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=eac6ac60b5e6
Flags: needinfo?(stomlinson)
https://hg.mozilla.org/mozilla-central/rev/eac6ac60b5e6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 65

4 years ago
I am re-opening this to have merged into Fx39. The last patch [1] merges cleanly into mozilla-aurora, a try push has been initiated [2].

[1] - https://bugzilla.mozilla.org/attachment.cgi?id=8601150
[2] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9927ccc413
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Target Milestone: mozilla40 → mozilla39
(Assignee)

Comment 66

4 years ago
I was informed that I used an incorrect process to get this uplifted to 39. Closing and following the correct process.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla39 → mozilla40
(Assignee)

Comment 67

4 years ago
Comment on attachment 8601150 [details] [diff] [review]
bug-1146904-sync-via-webchannel-012.patch

Approval Request Comment
[Feature/regressing bug #]: Enable Sync signin using WebChannels instead of CustomEvents - bug #1146904 

[User impact if declined]: Users will be unable to sign in to Sync from the new /firstrun flow

[Describe test coverage new/current, TreeHerder]: New xpcshell tests have been added to ensure WebChannel messages are received, dispatched, and handled as expected. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9927ccc413

[Risks and why]: low - Sync driven over WebChannels is an additive change to enable the new firstrun flow, the original about:accounts flow and functionality is left in place.

[String/UUID change made/needed]: none.
Attachment #8601150 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 68

4 years ago
Comment on attachment 8601150 [details] [diff] [review]
bug-1146904-sync-via-webchannel-012.patch

Approval Request Comment
[Feature/regressing bug #]: Enable Sync signin using WebChannels instead of CustomEvents - bug #1146904 

[User impact if declined]: Users will be unable to sign in to Sync from the new /firstrun flow

[Describe test coverage new/current, TreeHerder]: New xpcshell tests have been added to ensure WebChannel messages are received, dispatched, and handled as expected. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9927ccc413

[Risks and why]: low - Sync driven over WebChannels is an additive change to enable the new firstrun flow, the original about:accounts flow and functionality is left in place.

[String/UUID change made/needed]: none.
Attachment #8601150 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(Assignee)

Comment 69

4 years ago
Since Fx39 went from Aurora to Beta, I have changed the approval-mozilla-aurora request to approval-mozilla-beta.
Shane, why does this need to ship in 39?  If we don't uplift it to beta, is there a first run page already pointing to this for 39 and if so can we change that page so that we still use Custom Events?
Flags: needinfo?(stomlinson)
(In reply to Liz Henry (:lizzard) from comment #70)
> Shane, why does this need to ship in 39?  If we don't uplift it to beta, is
> there a first run page already pointing to this for 39 and if so can we
> change that page so that we still use Custom Events?

We are launching another improved firstrun page for Firefox 39.0 that will take advantage of this. See bug 1150222.
Flags: needinfo?(lhenry)
(Assignee)

Comment 72

4 years ago
> Shane, why does this need to ship in 39?  If we don't uplift it to beta, is there a first run page already pointing to this for 39 and if so can we change that page so that we still use Custom Events?

Hi :lizzard,
FirefoxAccountsCommand CustomEvents are only listened for in about:accounts [1], no pages other than about:accounts were ever able to listen for the CustomEvents. This patch enables a Sync sign-in when Firefox Accounts is embedded in web content, e.g., the improved firstrun page :cmore mentions. Without this patch, users will be required to open about:accounts to sign in to Sync instead of signing in directly from the first run page itself.

[1] - https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutaccounts/aboutaccounts.js#124
Flags: needinfo?(stomlinson)
Depends on: 1164658
(Assignee)

Comment 73

4 years ago
Hi Liz,
I have never requested an uplift before, are there any other prerequisites that need to be ticked before uplift can happen?
FWIW, I consider this a low-risk uplift.
Shane: I'd like for QE to verify this on 40 and then we can uplift it for beta 2 if we miss the beta 1 cutoff date.    Beta 1 will go to build on Friday and release Monday. Beta 2 will gtb next Thursday.  We have a lot of uplifted patches landing at once without much testing for the last two weeks, and so for anything that we can verify, it seems best to give verification a shot before uplift. 

How does that sound?
Flags: needinfo?(lhenry) → needinfo?(chrismore.bugzilla)
Andrei, can your team run through some Firefox Accounts and sync testing this week on 40?  We want to uplift this to 39 in either beta 1 or beta 2.
Flags: needinfo?(andrei.vaida)
Is there a test first run page we can use for testing this in 40 before we try the uplift to beta?
Flags: needinfo?(stomlinson)
(Assignee)

Comment 78

4 years ago
> Is there a test first run page we can use for testing this in 40 before we try the uplift to beta?

The firstrun page has not been created yet, but the same functionality can be tested using Fx 40 and visiting:

https://accounts.firefox.com/signup?service=sync&context=iframe
https://accounts.firefox.com/signin?service=sync&context=iframe

These URLs will cause FxA to use the account_updates WebChannel to communicate with Sync.

One possible surprise for testers: after the user signs in to FxA, no page transition occurs. This is expected behavior and will be updated on the FxA side once we flesh out the UX in more detail with the growth team. You will know the Sync sign in has occurred successfully by opening the hamburger menu or Sync prefs and seeing the user's email address has been updated.

Cases to test (though I suspect you have many more):
* Sign up for a new FxA account, verify same browser.
* Sign up for a new FxA account, verify different browser.
* Sign up for a new FxA account, check "Choose what to Sync". The user should see the "Choose what to Sync" dialog.
* Sign in to an existing account with a fresh profile.
* Sign in with two different accounts, i.e., sign in with account A, open Sync prefs & disconnect. Sign in with account B. The user should see the "Data will be merged" dialog.

Thanks Liz and Andrei.
Flags: needinfo?(stomlinson)
(In reply to Liz Henry (:lizzard) from comment #76)
> Andrei, can your team run through some Firefox Accounts and sync testing
> this week on 40?  We want to uplift this to 39 in either beta 1 or beta 2.

Assigning this to Catalin for verification.
Flags: needinfo?(andrei.vaida) → qe-verify+
QA Contact: catalin.varga
:lizzard: Is this uplift to 39 hinging on the verification of the links shane put in comment 78?

Andrei/Catalin: let us know asap on the verification as we need to get this in 39.0 to continue to improve the firefox first run experience on onboarding to fx accounts.
Flags: needinfo?(chrismore.bugzilla) → needinfo?(lhenry)
Chris: right now 38.0.5 has to be the priority for testing for folks at softvision but we can still get this into beta 2 next week.
Flags: needinfo?(lhenry)
I verified this fix using the following environment:

FF 40
Build Id: 20150525004005
OS: Win 7 x64, Ubuntu 14.04 x64,Mac Os X 10.9.5

During verification I've noticed an issue that I'm not sure is related with this fix:

- you can sign up to a new FF Account while you are logged in to a different FF account, doing this will log you to the newly created but the sync will not work. I've logged bug 1168087 related to this issue. Is this something related with this fix?
Flags: needinfo?(stomlinson)
(In reply to Catalin Varga [QA][:VarCat] from comment #82)
> I verified this fix using the following environment:
> 
> FF 40
> Build Id: 20150525004005
> OS: Win 7 x64, Ubuntu 14.04 x64,Mac Os X 10.9.5
> 
> During verification I've noticed an issue that I'm not sure is related with
> this fix:
> 
> - you can sign up to a new FF Account while you are logged in to a different
> FF account, doing this will log you to the newly created but the sync will
> not work. I've logged bug 1168087 related to this issue. Is this something
> related with this fix?

Thanks Catalin,
  I think this is acceptable - the page Shane sent you to was "unconditional", whereas the first-run page that will use this will only direct you to such a page if there's no user currently signed in.

So while that other bug is valid and should be addressed, I think we can call this verified if it works correctly when you are not already signed into a FxAccount (ie, while prefs->sync shows you as not logged in)
Flags: needinfo?(stomlinson)
Thanks for your quick response.

Verified the fix on:

FF 40
Build Id: 20150525004005
OS: Win 7 x64, Ubuntu 14.04 x64,Mac Os X 10.9.5
Comment on attachment 8601150 [details] [diff] [review]
bug-1146904-sync-via-webchannel-012.patch

I'd like this to ride with 40. It's still early in beta, but we need to limit uplifts to stability and security fixes, and fixes for regressions, whenever we can. We also have less time on the 39 beta cycle than we normally would because of the 38.0.5 release.
Attachment #8601150 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Liz Henry (:lizzard) from comment #85)
> I'd like this to ride with 40. It's still early in beta, but we need to
> limit uplifts to stability and security fixes, and fixes for regressions,
> whenever we can. We also have less time on the 39 beta cycle than we
> normally would because of the 38.0.5 release.

I'm really disappointed with this. The first uplift request was 15 days ago when 39 was (just) still on Aurora, the Firefox engineers judged it low risk, the risk is limited to Firefox Accounts and the marketing team has twice said it is important to them during that period, and we've repeatedly been told during that period it will make 39 - the last time just 3 days ago.
(In reply to Mark Hammond [:markh] from comment #86)

> I'm really disappointed with this. The first uplift request was 15 days ago
> when 39 was (just) still on Aurora, the Firefox engineers judged it low
> risk, the risk is limited to Firefox Accounts and the marketing team has
> twice said it is important to them during that period, and we've repeatedly
> been told during that period it will make 39 - the last time just 3 days ago.

OK, I'll give you some more of my context. This missed the train for 40. Uplifting it to aurora still seemed pretty reasonable. However, 39 beta 1 is still not released (because of 38.0.5) and normally it would have been in the beta channel 2 weeks ago. Since 15 days ago, there have been at least another 50 uplift requests. We don't have any crash data for those changes and it's a fair amount of change to land all at once. We intended to ship 39 today but from various issues discussed on release-drivers and in the channel meeting today, it will be delayed till at least Thursday. 

My concern is for the overall quality of Firefox 39. Uplifting features still in development to Beta should not be our priority. In fact, I don't think that something being important to marketing is a good reason to subvert the release process, which is in place to ensure the best quality release we can get to.  It's part of my job to make the call on uplifts. To do so I spent some reading back through the bug again today and did not feel good about uplifting given that we will have fewer beta cycles and need to focus on stability. However, I respect your judgement.  If you want to override this, we can figure out who gives the green light to do that; talk with Lawrence or Doug. You can also come to the channel meeting on Thursday.
Hi :lizzard - totally get that we're in a tight spot with the shortened 39 cycle.  Moreover, getting this out quickly is critical in iterative efforts toward understanding and testing our efforts to grow the base.  I'll ping Lawrence and :cmore on IRC tomorrow to request uplift.  I'm going to NI them both here.
Flags: needinfo?(lmandel)
Flags: needinfo?(chrismore.bugzilla)
There was a separate thread about this. I thought that we had agreed to defer this feature to 40 but perhaps I misunderstood. Let's clear this up as you suggest.
Flags: needinfo?(lmandel)
Hi :edwong. Obviously, I would love to get this out in fx 39 so we can keep collecting information on what is working and not working for the betterment of product growth. :lmandel and I have an email conversation where it seemed like fx 39 was not possible, but if there is a possibility that would make the growth team happy.

As for why we can't do this on the website only without product code... it is because functional websites that interact with the product and "do" instead of just "show" has provided higher conversion rates. The firstrun and whatsnew pages are one of a number of web experiences that blur the lines between web and product.
Flags: needinfo?(chrismore.bugzilla)
Sounds like this has been agreed to target fx40. Thanks everyone.
(Assignee)

Updated

4 years ago
See Also: → 1173688
Status: RESOLVED → VERIFIED

Updated

3 years ago
Depends on: 1270160

Updated

a year ago
Product: Core → Firefox
Target Milestone: mozilla40 → Firefox 40
You need to log in before you can comment on or make changes to this bug.