Closed Bug 1564131 Opened 5 months ago Closed 4 months ago

Theme is not synced

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 69+ fixed
firefox67 --- unaffected
firefox68 + wontfix
firefox69 + verified
firefox70 + verified

People

(Reporter: Ovidiu, Assigned: markh)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Affected versions

  • Tested on Beta 68 and Nightly 69.0a1(2019-07-07)
    Note: On FF Release 67.0.4 the issue is not reproducible.

Affected platforms

  • Tested on Mac Os X 10.14 and Windows 10

Steps to reproduce

  1. Open 2 profiles and sign in into sync with the same account on both profiles
  2. On one of the profiles select dark theme and sync
  3. On the second profile click on Sync button

Expected result

  • The theme is synced, the second profile has the dark theme.

Actual result

  • The theme is not synced, the second profile doesn't have the dark theme.

Regression range
This is a regression, please let me know if it's necessary to find out a regression range.

Nothing has changed recently in Sync which would explain this, so a regression would be much appreciated, thanks!

Flags: needinfo?(ovidiu.boca)

Here is the result:

Last good revision: 2127079505157362f2c8a54b8d2be4912c8c9806
72:13.29 INFO: First bad revision: ce04183ab703536f3c8957b93f13b41da10730ff
72:13.29 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2127079505157362f2c8a54b8d2be4912c8c9806&tochange=ce04183ab703536f3c8957b93f13b41da10730ff

Flags: needinfo?(ovidiu.boca)

Looks like a regression from bug 1525511.

Flags: needinfo?(kmaglione+bmo)
Regressed by: 1525511

Mark,

Can you please take a look at this change https://hg.mozilla.org/integration/mozilla-inbound/rev/ce04183ab703536f3c8957b93f13b41da10730ff that is part of the issue that caused the regression in syncing themes and see if there is something there that you can resolve?

Flags: needinfo?(markh)

I don't think this is a regression, as such. We stopped supporting LightweightThemes, which is apparently the only kind of theme that the theme sync code supported. The code that I removed wouldn't have worked for static themes.

Flags: needinfo?(kmaglione+bmo)

(In reply to Kris Maglione [:kmag] from comment #5)

I don't think this is a regression, as such. We stopped supporting LightweightThemes, which is apparently the only kind of theme that the theme sync code supported. The code that I removed wouldn't have worked for static themes.

Can you please give more information here? Sync supports static themes via addon syncing, and I still see things like "dark theme" available as a theme, and as this bug reports, we apparently no longer sync when that theme is enabled - so it's clearly a regression.

It's a shame you didn't have someone from the sync team review the sync patches as this could have been avoided, but I think something needs to be done here to restore the functionality that's been lost.

Flags: needinfo?(markh) → needinfo?(kmaglione+bmo)

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

(In reply to Kris Maglione [:kmag] from comment #5)

I don't think this is a regression, as such. We stopped supporting LightweightThemes, which is apparently the only kind of theme that the theme sync code supported. The code that I removed wouldn't have worked for static themes.

Can you please give more information here? Sync supports static themes via addon syncing, and I still see things like "dark theme" available as a theme, and as this bug reports, we apparently no longer sync when that theme is enabled - so it's clearly a regression.

If static theme syncing works via add-on syncing then nothing should have changed. I removed the code which touches the "lightweightThemes.selectedThemeID" preference and the related LightweightThemeManager properties which are no longer used for anything.

Flags: needinfo?(kmaglione+bmo)

(In reply to Kris Maglione [:kmag] from comment #7)

If static theme syncing works via add-on syncing then nothing should have changed.

I'm not quite with you - something clearly did change right? And we agree that the change was caused by bug 1525511, right? And it worked before that bug and failed to work after, so it's clearly a regression, right?

Is there any help you can offer to work out what we do to resolve this?

Flags: needinfo?(kmaglione+bmo)

Presumably the problem is that sync only syncs add-ons that are in the user profile, which is not the case for built-in themes. So while the old persona sync logic likely worked for them while they were still personas, the static theme logic doesn't.

Flags: needinfo?(kmaglione+bmo)

(Which is to say, removing the dead persona sync logic didn't cause this, migrating the built-in themes to static themes did)

Assignee: nobody → markh
Attachment #9077619 - Attachment description: Bug 1564131 - re-enable syncing of builtin themes. r?aswan → Bug 1564131 - re-enable syncing of builtin themes. r?rpl

Hey Mark, want to land this one?

Flags: needinfo?(markh)
Pushed by mhammond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/024783b4dcd4
re-enable syncing of builtin themes. r=rpl
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Flags: needinfo?(markh)

Comment on attachment 9077619 [details]
Bug 1564131 - re-enable syncing of builtin themes. r?rpl

Beta/Release Uplift Approval Request

  • User impact if declined: The user's choice of default theme will not be synced
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Risk is limited to preference syncing in sync
  • String changes made/needed: None
Attachment #9077619 - Flags: approval-mozilla-beta?
Flags: in-testsuite+

Comment on attachment 9077619 [details]
Bug 1564131 - re-enable syncing of builtin themes. r?rpl

Fixes syncing of builtin themes. Approved for 69.0b7.

Attachment #9077619 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

This is minor enough that I don't think we need to worry about it for an Fx68 dot release. Did you want to nominate this for ESR68 uplift, however, Mark?

Flags: needinfo?(markh)

Comment on attachment 9077619 [details]
Bug 1564131 - re-enable syncing of builtin themes. r?rpl

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: A regression for how themes are synced
  • User impact if declined: Default themes not synced
  • Fix Landed on Version: 69, 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk limited to sync
  • String or UUID changes made by this patch: None
Flags: needinfo?(markh)
Attachment #9077619 - Flags: approval-mozilla-esr68?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

I verified this on Mac OS X 10.14 and Windows 10 with FF Nightly 70.0a1(2019-07-22) and Beta 69.0b6, Release 68 and Firefox Release 67 and I have different behaviors:

This are the steps:

  1. Open 2 profiles
  2. Log with your sync account in the first profile and set a theme (I use a custom one) - click on "Sync Now" button
  3. Log in with your sync account (the same as in step 1) in the second profile and wait a minute.

Results depending on the Firefox version:

FF Nightly 70.0a1(2019-07-22), Beta 69.0b6 and Release 68 - the theme from the first profile is not synced on the second profile(everything else is sync- bookmarks, addons). Now you are in the stage where the profile 1 has the theme set by you and the profile 2 has the default theme. If you select profile 2 and click on the "Sync Now" button and then you go to the profile 1 and you click on the "Sync Now" button the theme from profile 2 is imported into profile 1, now if I do once again the steps from the description everything will work as expected and the theme from profile 1 will be synced with profile 2.

FF Release 67 - after step 3 from the description the theme from profile 1 is automatically set to profile 2.

Mark, please take a look at this an let me know what is your opinion. Thanks

Flags: needinfo?(markh)

Sadly there's an old issue whereby if the theme is changed before sync initializes - which may be ~10 seconds after the browser start - we will miss the change. The easiest way to check this is to ensure you perform a "sync now" before changing the theme, then one after the change. Can you please verify that this explains what you see?

Flags: needinfo?(markh) → needinfo?(ovidiu.boca)

I verify it as you suggested and it works, but on FF 67 release the theme was imported on the second profile without performing any sync before. From the user experience, I don't think this is working as it should.

Also, I tested FF 68 ESR and now the behaviour is the same as on FF 67, the theme is imported on the second profile immediately after the sync is performed without doing any additional steps(perform sync before I change the theme and perform the second sync after the theme was changed.) If this will be uplifted to ESR the behaviour will be changed and will cause a regression.

Please let me know if the expected result is the one from comment 21 and I will mark the flags accordingly.

Flags: needinfo?(ovidiu.boca)

This code isn't going to work for syncing changes for these themes between versions 67- and 68+. The themes are implemented differently in those versions, and the preferences that sync uses to detect when they're activated don't match across versions.

Honestly, this should all have been done using AddonManager APIs rather than preferences to begin with, since those don't care how a theme is implemented underneath, and would handle any changes like this transparently. But there's no fixing that for users of 67 at this point. The only way to deal with that issue would be for the sync engine to try to mirror changes in the new preference to the old one, and vice versa. I'm not sure it's worth the effort.

I verified this on Mac OS X 10.14, Windows 10 x64 with FF Nightly 70.0a1(2019-07-23) and FF beta 69.0b6 and if I follow the steps from comment 21 (not from the description) the issue is not reproducible.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

Comment on attachment 9077619 [details]
Bug 1564131 - re-enable syncing of builtin themes. r?rpl

Thanks for the verification, Ovidiu. Approved for 68.1esr also.

Attachment #9077619 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.