1791478 - SyncedTabs.jsm should better handle the primary password being recently unlocked.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

STR:

• Start with PP enabled, decline the first PP prompt.
• Note that as expected, the account menu has no synced tabs
• Arrange for the PP to be unlocked through some other mechanism than forcing a sync (eg, trying to autofill a password or similar should do this, but executing Weave.Utils.ensureMPUnlocked() in the browser console is good for testing)

Expected:

• Showing the account menu should start a sync
  Actual:
• It doesn't - the user either needs to force a sync or wait for the next scheduled sync.

SyncedTabs.jsm explicitly declines trying to sync by looking at the Weave.Status.login state and this state isn't updated when the PP is unlocked outside of sync. A reasonable solution might be to undo that check (added in bug 1789341 and tweaked in bug 1789876) and replace it with an explicit "is pp locked" check, or maybe something else?

Comment 1

[:Gijs (away/busy until Mar 10)]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #0)

SyncedTabs.jsm explicitly declines trying to sync by looking at the Weave.Status.login state and this state isn't updated when the PP is unlocked outside of sync.

Are there other similar cases where we avoid the new sync? I wonder if a similar situation is related to the long-lasting "loading..." state people were seeing in bug 1787975 and friends.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs (he/him) from comment #1)

Are there other similar cases where we avoid the new sync? I wonder if a similar situation is related to the long-lasting "loading..." state people were seeing in bug 1787975 and friends.

I don't think so, but I guess it is possible - we insist on the "login status" being in one of 2 states or we don't sync - which might be causing us to not sync when we should try even if it fails - I don't think it is though, but it's possible. Replacing those checks with "is the PP locked" is easier to rationalize about, but might cause us to try and fail to sync more often. But that was true a few weeks ago and didn't cause any problems other than forcing the PP prompt each time.

IOW, the only reason a change was made here was due to that PP prompt, so just just explicitly checking that PP state seems sensible and should have fewer unintended consequences.

Comment 3

[Sammy Khamis [:skhamis]]

Attachment: Bug 1791478 - SyncedTabs should check if pp is locked before syncing r?markh,sfoster

Comment 4

[Sammy Khamis [:skhamis]]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #2)

(In reply to :Gijs (he/him) from comment #1)

Are there other similar cases where we avoid the new sync? I wonder if a similar situation is related to the long-lasting "loading..." state people were seeing in bug 1787975 and friends.

I don't think so, but I guess it is possible - we insist on the "login status" being in one of 2 states or we don't sync - which might be causing us to not sync when we should try even if it fails - I don't think it is though, but it's possible. Replacing those checks with "is the PP locked" is easier to rationalize about, but might cause us to try and fail to sync more often. But that was true a few weeks ago and didn't cause any problems other than forcing the PP prompt each time.

IOW, the only reason a change was made here was due to that PP prompt, so just just explicitly checking that PP state seems sensible and should have fewer unintended consequences.

This felt like an easy change and I agree with Mark, it drastically reduces the amount of unintended consequences this could have (since login could also be in other states besides the two we made). Tested this and it works surprisingly well but added everyone as review to get more thoughts on it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1b1117e0058
SyncedTabs should check if pp is locked before syncing r=markh,sfoster

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d1b1117e0058