558191 - Theme/persona sync sometimes doesn't happen until restart of client

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Tracy Walker [:tracy]]

This one is not reliably reproducible at all times. The following are steps I think most often can reproduce the bug:

1) set up two clients on separate machines each with the default theme and one persona.  (I've been trying back and forth between XP and Mac 10.6 with Fx 3.6.3. I've been testing with the Dragon Flower persona, if it makes any difference.
2) Have both machines running in their default theme
3) Shut down and restart both machines.  
4) On client A, use the persona theme
5) Sync client A to server
6) Sync client B

test results:
The persona on client B does not dynamically update as Preferences are synced. However, go to the Add-ons Manager and notice that the expected persona is selected as in use.

expected results: 
The persona on client B updates and is displayed in the browser once preferences have been synced.

Comment 1

[Mike Connor [:mconnor]]

If you open a second browser window, is the correct persona applied then?

Comment 2

[Mike Connor [:mconnor]]

Not going to block on this, it's not likely to be a regression since 1.1 since we haven't changed this code.

Can you also replicate this with two personas, or is it only with exactly one persona?  I suspect it's the latter, and we're tripping up sometimes when hitting http://hg.mozilla.org/labs/weave/file/tip/source/modules/engines/prefs.js#l150, since if there isn't a current persona installed, and the pref setting hasn't gone through, we'll fail that check, despite having the right prefs set.

I think that pref is meant to be a check for personas-are-enabled, but I think we can/should fix that check to check the isSelected pref and set currentTheme unconditionally.

Comment 3

[Tracy Walker [:tracy]]

(In reply to comment #1)
> If you open a second browser window, is the correct persona applied then?

Yes, the correct theme is applied in a New Window.

(In reply to comment #2)
> Can you also replicate this with two personas, or is it only with exactly one
> persona?  

Added a second persona. synced it to second machine.  Then followed STR's.  Same problem; theme isn't updated in current window.

Note: It's only buggy switching in or out of the default theme.  Switching between two personas doesn't exhibit this bug.

Comment 4

[Mike Connor [:mconnor]]

We should just set currentTheme unconditionally, instead of checking it, to fix this.

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> We should just set currentTheme unconditionally, instead of checking it, to fix
> this.
Does that sound right?

I can't seem to reproduce this issue, but I haven't tested on Windows yet.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (In reply to comment #4)
> > We should just set currentTheme unconditionally, instead of checking it, to fix
> > this.
> Does that sound right?

I think you need to narrow this down further what is going on here. I believe the suggestion is to change from this:

> if (ltm.currentTheme) {
>   ltm.currentTheme = null;
>   ltm.currentTheme = ltm.usedThemes[0];
> }

To:

> if (this._prefs.setBoolPref("lightweightThemes.isThemeSelected")) {
>   ltm.currentTheme = null;
>   ltm.currentTheme = ltm.usedThemes[0];
> }

I can't imagine a situation where that is going to be any better since ltm.currentTheme returns ltm.usedThemes[0] if "lightweightThemes.isThemeSelected" is true.

Comment 7

[Ed Lee :Mardak]

Looking a bit deeper, it seems like currentTheme is returning undefined, so perhaps isThemeSelected hasn't been synced yet. So perhaps the better fix is to do the currentTheme check /after/ all the prefs have synced?

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Looking a bit deeper, it seems like currentTheme is returning undefined, so
> perhaps isThemeSelected hasn't been synced yet. So perhaps the better fix is to
> do the currentTheme check /after/ all the prefs have synced?

Yeah the code you have won't work unless both isThemeSelected and usedThemes preferences have been synced

Comment 9

[Ed Lee :Mardak]

Attachment: v1

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 440590 [details] [diff] [review]
v1

ah, race conditions, how I love thee.  Or something.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/76fb8e093c8c
Wait until all lightweight theme prefs have synced before poking at the lightweight theme manager.

Comment 12

[Tracy Walker [:tracy]]

I'm still seeing this bug on 1.3b1. Though now it only shows when switching from a persona back to default. As in:

1)have two machines synced to the same persona.
2) on client A. switch theme back to default, Sync
3) Sync client B.  

Test results: 
Initial window not updated to default theme.  (subsequent windows or on restart displays default theme)

Expected result:
Dynamic update to default theme

Comment 13

[Mike Connor [:mconnor]]

Tracy, can you file a separate bug?  It's related, but it's a separate bug, IMO.

Comment 14

[Tracy Walker [:tracy]]

Ok,  Persona syncs dynamically but not default theme.  new bug to follow