Closed Bug 699175 Opened 8 years ago Closed 8 years ago

The current theme is not active when updating from Firefox 7.0b6 to Firefox 8.0b6

Categories

(Toolkit :: Add-ons Manager, defect, major)

8 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox8 - ---
firefox9 + ---
firefox10 + verified

People

(Reporter: simona.marcu, Assigned: mossop)

References

Details

(Keywords: regression, verified-beta, Whiteboard: [8b6][qa+][qa!:10])

Attachments

(7 files, 1 obsolete file)

Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0

The current theme is not kept as default when updating from Firefox 7 beta 6 to Firefox 8 beta 6.

Reproducible: always

Steps to reproduce:
1. Install Firefox 7.0 beta 6
2. Modify the update channel to "releasetest"
3. Install a theme of your choice that is compatible with Firefox 7.0b6 and set that theme to default and restart.
4. Go to Help menu -> About Firefox and perform the update.

Actual results:
The theme installed in step 3 is not set as default after performing the update.

Expected result:
The theme installed in step 3 is set as default after the update.
OS: Windows XP → All
Whiteboard: 8.b6
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: theme → add-ons.manager
What theme were you testing with?

Can you enable extensions.logging.enabled and then try again and after the update copy messages from the error console to this report.
Whiteboard: 8.b6 → [8.b6]
Is this new to 8.0? Does the same thing happen going from 6 to 7? How about from 4 to 5?
I just tested this on OSX updating from Firefox 7.0.1 with Nasa Night Launch installed and active. The "select your add-ons" UI is themed by the default theme, this is expected since extensions aren't active at that point, but then once the app proper starts it is correctly themed by Nasa Night Launch.

I don't have windows handy to test right now to see if this is platform specific but we need more details to be able to try to reproduce this.
I could reproduce using the steps in comment 0 on Ubuntu 11.10 and Mac OS 10.6. I used both Nasa Night Launch and Lava Fox v1 themes.

The installed theme (listed in Appearance menu in Add-On Manager) was disabled when upgrading to F8Beta6 through the "releasetest" channel. I've tried to manual update from Firefox 7.0.1 to Firefox8Beta6 and the issue could be reproduced. 

(In reply to Dave Townsend (:Mossop) from comment #1)
> What theme were you testing with?
> 
> Can you enable extensions.logging.enabled and then try again and after the
> update copy messages from the error console to this report.

I will attach the results from Ubuntu in a text file. 



(In reply to Christian Legnitto [:LegNeato] from comment #2)
> Is this new to 8.0? Does the same thing happen going from 6 to 7? How about
> from 4 to 5?

I couldn't reproduce while updating from Firefox 6 to 7 on Ubuntu 11.10 using the steps from comment 0. The theme was enabled after the upgrade.
Hardware: x86 → All
Whiteboard: [8.b6] → [8b6]
Uh, is that related to the exception thrown by Testpilot and an early exit of the add-on code?

Exception calling callback: [Exception... "'[JavaScript Error: "self._interfaceBuilder is undefined" {file: "resource://testpilot/modules/setup.js"

I would think that this only happens for beta releases and test pilot installed. Virgil, could you please prepare a profile and set 'extensions.installDistroAddons' to false to disallow the installation of test pilot, and try again?
Tried modifying the value to false but test pilot is still installed when upgrading. Should I flip any other pref for this to work correctly?

Anyways, simply removing feedback from add-ons does the trick-Test Pilot no longer appears after upgrading. But the theme is still disabled after upgrading. 

I've attached the messages from the terminal on Ubuntu 11.10 after doing this.
Sorry, forgot to ask. Could you also attach extensions.sqlite after closing Firefox after the update.
Just re-tested with Lavafox and I'm still not seeing this for some reason. What does the "Select your add-ons" UI look like during this? Can I get a screenshot?
Added the screenshot for Add-on Selection UI. Just the normal UI for me, it isn't customized with the theme installed. 

Attaching the extensions.sqlite too. Used Lava Fox theme.

We can reproduce this issue here on different machines with different OSs. Happens every time.
Attachment #571900 - Attachment mime type: text/plain → application/octet-stream
Frustratingly I still can't reproduce this or see any reason why it would be happening. Does the same happen if you just run Firefox 8 after running Firefox 7 (not doing the auto-update just downloading it manually). How about Firefox 10 after Firefox 7? If it happens for either of those cases then I can create a try build with some additional logging to try to figure this out.
Bah, should have looked into tracking this earlier. However my take is that this isn't bad enough to block Firefox 8 at this point, we haven't identified any way to resolve it either but switching the theme by accident (and apparently not for all users) doesn't seem worth holding the release or even respinning an RC at this point.
Yeah, no worries, we've been watching it since QA flagged it.
(In reply to Dave Townsend (:Mossop) from comment #12)
> Frustratingly I still can't reproduce this or see any reason why it would be
> happening. Does the same happen if you just run Firefox 8 after running
> Firefox 7 (not doing the auto-update just downloading it manually). How
> about Firefox 10 after Firefox 7? If it happens for either of those cases
> then I can create a try build with some additional logging to try to figure
> this out.

Yes, used a new profile in 7.0.1 (Ubuntu 11.10 and Windows XP), installed Nasa theme (enabled). When using the same profile in Firefox 8 Latest Beta, the theme was disabled automatically. The same occurs for Firefox 10. 

I've set extensions.shownSelectionUI to true, just to rule out an issue with the start-up interface, and the issue is reproducible regardless of this.

Will be happy to check the try build to figure this out.
I made some builds with some additional logging, they are available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dtownsend@mozilla.com-0ee9a387e1bf/
(In reply to Dave Townsend (:Mossop) from comment #16)
> I made some builds with some additional logging, they are available here:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dtownsend@mozilla.
> com-0ee9a387e1bf/

Issue is not reproducible on the try builds.
Installed Nasa Night theme or LavaFox theme on Firefox 7.0.1. Run the same profile on the try builds and the themes were not disabled. Tested on Windows XP, Mac OS X 10.6 and Ubuntu. 

Also, I noticed that if I install Nasa Night theme on the latest Nightly and I run the same profile on Firefox 7.0.1 the theme is active, but if I run again on the same profile with the latest Nightly the theme is disabled.
Well that's frustrating, there is basically no difference between those try builds and the nightly builds, except for logging so I don't know why they wouldn't also cause the problem.
Here's what this bug has caused with one theme that had about 40,000 active users before the FF8 'upgrade':

http://www.game-point.net/misc/ff8-theme.png

Nice.
(In reply to Jeremy Morton from comment #19)
> Here's what this bug has caused with one theme that had about 40,000 active
> users before the FF8 'upgrade':
> 
> http://www.game-point.net/misc/ff8-theme.png

This is a drop of nearly half of the users for this theme. :( CC'ing some people to get an impression how this plays into our 8.0 strategy.
Severity: normal → major
Last good nightly: 2011-11-01
First bad nightly: 2011-11-02

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-11-01&enddate=2011-11-02

Manually upgraded from Firefox 9Beta1 to Nightlies.
Last good nightly: 2011-09-27
First bad nightly: 2011-09-28

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-09-27&enddate=2011-09-28

When updating from Firefox 8 to Nightlies, a different regression range was found.
[Triage Comment]
We discussed this yesterday afternoon and agreed that it is not within the scope of an 8.0.1 chemspill.
These regression ranges don't really make much sense as the ranges are all for code that landed in Firefox 9 or 10, but we know the problem occurs with Firefox 8 so there must be something earlier than this. Testing the upgrade from Firefox 7 to nightly builds of Firefox 8 might reveal a better regression range here.
Upgraded from Firefox 7.0.1 to different Firefox 8 branches using Lavafox theme. Seems that this got broke with Firefox8Beta5. Couldn't find any F8 Nightly that could reproduce this issue. 
This is the changelog for Firefox8Beta5: http://hg.mozilla.org/releases/mozilla-beta/log/76391

In F8Beta4, in the confirm add-on dialog, the theme is also listed. Starting with Beta5, the dialog no longer displays the theme.

3 add-on related bugs in the changelog for Beta5:
Bug 693698: 3rd Party add-ons from the application folder are not disabled on upgrade. r=Unfocused, a=LegNeato 
Bug 694595: Some settings for add-ons aren't being migrated when we change database schemas. r=Unfocused, a=LegNeato 
Bug 693743: Some 3rd party add-ons are getting installed into the profile and are not disabled on start-up. r=Unfocused, a=LegNeato
Tried upgrading from 7.0.1 to Nightly from the dates on which the three bugs were landed, in order to try and figure this out, but the results are different, or the theme is simply displayed inaccurately as incompatible with 10 after the upgrade and thus disabled. Hope this helps to figure this out.
Finally managed to reproduce this and I think I can see the problem, though from what I'm seeing I don't understand how it isn't 100% reproducible.
Blocks: 693743
Assignee: nobody → dtownsend
Attached patch patch rev 1 (obsolete) — Splinter Review
The patch for bug 693743 was broken in some unfortunate ways. It set foreignInstall to true even when add-ons already existed previously and we were just reloading them because of a schema change in the database. It also would set userDisabled to true for these. We didn't really notice this because I didn't write thorough enough tests for the new foreignInstall property and we later applied migrated data, so normal extensions didn't all get disabled. But themes don't get userDisabled migrated which is why they are getting disabled by the upgrade. I still have no clue why this isn't 100% reproduceable though :s

To solve the main problem here I've moved the code around in addMetadata a bit and created a couple of useful variables for testing what situation it is getting called for. So it now only sets foreignInstall for cases where it really is a new install and only disables in that case too.

The downside is that now the values for foreignInstall that users have in their databases are useless. They are likely all true except for anything they've installed since installing 8.0. This field I'd much rather be incorrectly false than incorrectly true so I've also renamed the field in the database so everyone's will get reset to false, however I've also just forced anything outside the profile to be true (apart from the default theme).

Renaming the field but not the API property is sort of a pain, I could use the new name throughout XPIProvider.jsm but just do the rename in AddonWrapper, or I could attempt to rename at all the points we add/remove from the DB. Instead though it seems nicer to just make both properties work on AddonInternal, the database code can read and write the new externalInstall property while the other code can still use foreignInstall. Maybe you disagree though Blair?
Attachment #575587 - Flags: review?(bmcbride)
Almost forgot, I bump DB_SCHEMA to 11 here for trunk, when I land on aurora I'll use schema 10. If we decide we want this on beta too then I guess I'd have to use 12 there and remember to bump trunk to 13 next time or something.
(In reply to Dave Townsend (:Mossop) from comment #29)
> Almost forgot, I bump DB_SCHEMA to 11 here for trunk, when I land on aurora
> I'll use schema 10. If we decide we want this on beta too then I guess I'd
> have to use 12 there and remember to bump trunk to 13 next time or something.

If there's *any* chance this could land on beta, it seems easier to just bump trunk to 12 and aurora to 11 right now. Will save a lot of hassle later on, and we're not gonna overflow schemaVersion any time soon (I checked: it's a 32bit signed integer).
Comment on attachment 575587 [details] [diff] [review]
patch rev 1

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +155,5 @@
>                              "releaseNotesURI",
>                              "applyBackgroundUpdates"];
>  const DB_BOOL_METADATA   = ["visible", "active", "userDisabled", "appDisabled",
>                              "pendingUninstall", "bootstrap", "skinnable",
> +                            "softDisabled", "externalInstall",

I still don't like having externalInstall/onExternalInstall be related to completely different things :\ Any objections to isForeignInstall?

@@ +7076,5 @@
> +  },
> +  set externalInstall(aVal) {
> +    this.foreignInstall = aVal;
> +  },
> +

Yea, this works.

::: toolkit/mozapps/extensions/test/xpcshell/test_migrate2.js
@@ +3,5 @@
>   */
>  
>  // Checks that we migrate data from future versions of the database
> +// Note that since the database doesn't contain the foreignInstall field we
> +// should just assume that no add-ons were foreignInstalls

Is is possible to test addons not in the profile scope?
Attachment #575587 - Flags: review?(bmcbride) → review-
Could I ask what this patch is actually intended to do?
(In reply to Jeremy Morton from comment #32)
> Could I ask what this patch is actually intended to do?

Primarily to make it so the active theme is retained across application updates
Attached patch patch rev 2Splinter Review
Updated
Attachment #575587 - Attachment is obsolete: true
Attachment #575993 - Flags: review?(bmcbride)
Blocks: 702506
Attachment #575993 - Flags: review?(bmcbride) → review+
Pushed to inbound. I'll be requesting approval for aurora and beta once it's had some testing on trunk (and I've made a patch that actually applies there).

https://hg.mozilla.org/integration/mozilla-inbound/rev/47c587d65242
Though I think the chances of this happening are infinitesimal, let's throw the idea out there anyway.  As it's clear that huge numbers of people using custom themes have had them disabled because of this bug, and probably don't even realize that the theme is still compatible with Firefox and therefore won't think to re-enable it, why not pop up a dialog on first run of Firefox 9 asking whether the user wants to enable a custom theme, if they have any themes other than default installed?  :-)

Well, it was worth a try...
https://hg.mozilla.org/mozilla-central/rev/47c587d65242
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Whiteboard: [8b6] → [8b6][qa+]
Attached patch aurora patchSplinter Review
This is the same patch backported to aurora. We have another schema change there so unless we take this too we'll see users switch back to the default theme again when they upgrade to Firefox 10.
Attachment #578074 - Flags: approval-mozilla-aurora?
Attached patch beta patchSplinter Review
And this is the same for beta. As there is no schema change there this isn't strictly necessary, I'm considering requesting approval for it anyway first for users upgrading direct from Firefox 6 to 8 and second because it will correct and start building the foreignInstall field in the database sooner.
Comment on attachment 578074 [details] [diff] [review]
aurora patch

[Triage Comment]
Approving for Aurora in preparation for consideration for beta, but it's very unlikely we'll take on beta with only one build remaining.
Attachment #578074 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/09ccc6c2cb3a

I think we'll skip beta, I mainly made the patch in case we ended up taking some of the DTC stuff on beta and changing the schema there, without that there is low risk that this will happen with upgrades to 9.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0

Verified on Ubuntu 11.10, Mac OS 10.6, Windows XP in Firefox 10 Beta1. 
1. Start any Firefox 9 beta (e.g. Firefox 9Beta 5)
2. Install a theme (e.g. Lavafox, Walnut) and enable it.
3. Select About Firefox and upgrade to Firefox 10Beta1.

The theme is active after the upgrade.
Keywords: verified-beta
Whiteboard: [8b6][qa+] → [8b6][qa+][qa!:10]
You need to log in before you can comment on or make changes to this bug.