Last Comment Bug 655254 - When an add-on moves to a different directory but doesn't change in any other way we should keep updated compatibility information
: When an add-on moves to a different directory but doesn't change in any other...
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: ARM Android
P1 major with 1 vote (vote)
: mozilla7
Assigned To: Dave Townsend [:mossop]
: Andy McKay [:andym]
Depends on: 744833
Blocks: 657019
  Show dependency treegraph
Reported: 2011-05-06 07:33 PDT by Cristian Nicolae (:xti)
Modified: 2012-05-11 08:49 PDT (History)
11 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Disabled addons (51.43 KB, image/png)
2011-05-06 07:33 PDT, Cristian Nicolae (:xti)
no flags Details
extensions.sqlite (384.00 KB, application/x-sqlite3)
2011-05-10 02:07 PDT, Cristian Nicolae (:xti)
no flags Details
extensions.sqlite-journal (224.55 KB, application/octet-stream)
2011-05-10 02:08 PDT, Cristian Nicolae (:xti)
no flags Details
patch rev 1 (16.15 KB, patch)
2011-05-27 13:49 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
patch rev 2 (16.18 KB, patch)
2011-06-03 14:24 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description User image Cristian Nicolae (:xti) 2011-05-06 07:33:51 PDT
Created attachment 530617 [details]
Disabled addons

Build id : Mozilla/5.0 (Android;Linux armv7l;rv:6.0a1)Gecko/20110506
Firefox/6.0a1 Fennec/6.0a1
Device: HTC Desire
OS: Android 2.2

Steps to reproduce:
1. Open Fennec App
2. Install at least 1 add-on
3. Quit Fennec
4. Move Fennec App to SD Card
5. After the app is moved reopen it
6. Go to Addons Manager

Expected result:
After step 6, addons installed at step 2 are still active and user is able at least to disable them.

Actual result:
Addons are disabled and buttons "Options" and "Enable" are grayed out. User is able only to uninstall those addons.

If after step 5, are repeated steps 2-3 and then the app is moved back to internal memory, all the installed addons are disabled.
Please see the attached screenshot.
Comment 1 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-05-06 07:38:23 PDT
Dave - I am working under the assumption the when an add-on is installed, we remember the absolute install path. When we move the profile, the install path is no longer valid.

If this is true, is there a way to "rebase" the add-ons?
Comment 2 User image Dave Townsend [:mossop] 2011-05-06 08:37:14 PDT
The code should be detecting this. Can you enable extensions.logging.enabled and get a log of the text console of the first startup after the app is moved?
Comment 3 User image Cristian Nicolae (:xti) 2011-05-09 01:24:19 PDT
(In reply to comment #2)
> The code should be detecting this. Can you enable extensions.logging.enabled
> and get a log of the text console of the first startup after the app is
> moved?

I've installed 3 add-ons (Quit Fennec, Personas and Nightly Tester Tools) and here is the log just after I've moved the app to SD card:

LOG addons.xpi: Writing add-ons list
LOG addons.xpi: Updating add-ons states
LOG addons.xpi: Updating database with changes to installed add-ons
LOG addons.xpi: Add-on modified in app-profile
LOG addons.xpi: Add-on modified in app-profile
Blocklist::_loadBlocklist: no XML File found
LOG addons.xpi: Add-on modified in app-profile
LOG addons.xpi: Opening database
LOG addons.xpi: checkForChanges
LOG addons.xpi: startup
Could not read chrome manifest file '/data/data/org.mozilla.fennec/chrome.manifest'.
Warning: Cannot load binary components from a jar.
Source File: /mnt/asec/org.mozilla.fennec-1/pkg.apk:components/components.manifest Line: 35
Comment 4 User image Cristian Nicolae (:xti) 2011-05-09 05:51:38 PDT
After I've updated the app with the latest build (20110509), there is one more warning about addons:
Warning: WARN addons.repository: Unknown type id when parsing addon: 3
Source File: resource://gre/modules/AddonRepository.jsm
Comment 5 User image Dave Townsend [:mossop] 2011-05-09 09:58:40 PDT
Strange, the log suggests that it spotted that the add-ons had moved. Can you attach a copy of extensions.sqlite from the profile before and after the move?
Comment 6 User image Dave Townsend [:mossop] 2011-05-09 11:39:28 PDT
Actually I can see this happening now. Looks like on the second startup on the sdcard things start working again?
Comment 7 User image Dave Townsend [:mossop] 2011-05-09 12:36:09 PDT
I think I know what is going on here. All of the add-ons I've found to test are marked compatible up to 4.0.* in their install.rdf but have been bumped to 6.0.* on AMO which is why they install ok. When an add-on has changed directory on startup we basically throw away the updated compatibility info and just use the facts from install.rdf.
Comment 8 User image Dave Townsend [:mossop] 2011-05-09 13:21:14 PDT
Ok, confirmed this is the case. I guess if the add-on keeps the same version number then we probably should keep the compatibility info in the database
Comment 9 User image Cristian Nicolae (:xti) 2011-05-10 02:07:53 PDT
Created attachment 531272 [details]

I've attached the extensions.sqlite after I've moved the app to SD Card, but I do not have access to device /data/. The device should be rooted, but I cannot do that.
There is another file on SD Card "extensions.sqlite-journal". I will attach it too. Maybe it would be helpful.
Comment 10 User image Cristian Nicolae (:xti) 2011-05-10 02:08:55 PDT
Created attachment 531273 [details]
Comment 11 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-05-13 12:18:10 PDT
Dave - what kind of timeframe ETA do you have for this bug?
Comment 12 User image Dave Townsend [:mossop] 2011-05-27 13:49:44 PDT
Created attachment 535740 [details] [diff] [review]
patch rev 1

This fix works for the case we are concerned with. The install location has moved and so the extension's directory has changed but nothing else about it has. Fennec's code for moving the profile to the sdcard takes steps to ensure the file modification times remain the same so we rely on that. This includes a test and I've also build this for android and tested it manually to verify it works.
Comment 13 User image Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-27 15:09:03 PDT
Comment on attachment 535740 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -2168,17 +2168,17 @@ var XPIProvider = {
>    * @param  aMigrateData
>    *         an object generated from a previous version of the database
>    *         holding information about what add-ons were previously userDisabled
>    *         and updated compatibility information if present
>    * @param  aActiveBundles
>    *         When performing recovery after startup this will be an array of
>    *         persistent descriptors of add-ons that are known to be active,
>    *         otherwise it will be null
>-   * @return true if a change requiring a restart was detected
>+   * @return true if a change requiring flushing the caches was detected
>    */
>   processFileChanges: function XPI_processFileChanges(aState, aManifests,
>                                                       aUpdateCompatibility,
>                                                       aOldAppVersion,
>                                                       aOldPlatformVersion,
>                                                       aMigrateData,
>                                                       aActiveBundles) {
>     let visibleAddons = {};
>@@ -2192,17 +2192,17 @@ var XPIProvider = {
>      *
>      * @param  aInstallLocation
>      *         The install location containing the add-on
>      * @param  aOldAddon
>      *         The AddonInternal as it appeared the last time the application
>      *         ran
>      * @param  aAddonState
>      *         The new state of the add-on
>-     * @return true if restarting the application is required to complete
>+     * @return a boolean indicating if flushing caches is required to complete
>      *         changing this add-on
nit: just noticed this. You use "@return true..." above and "@return a boolean..." here. I'd prefer consistency throughout the file.
Comment 14 User image Cristian Nicolae (:xti) 2011-06-01 09:24:42 PDT
This issue is reproducing on Beta 5:
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:5.0)Gecko/20110527
Firefox/5.0 Fennec/5.0
Device: LG Optimus 2X
OS: Android 2.2
Comment 15 User image Dave Townsend [:mossop] 2011-06-03 14:24:19 PDT
Created attachment 537233 [details] [diff] [review]
patch rev 2

Corrected the comment
Comment 16 User image Dave Townsend [:mossop] 2011-06-10 14:18:46 PDT
Comment 17 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2011-06-23 06:42:53 PDT
Cristian, can you please verify this fix?
Comment 18 User image Cristian Nicolae (:xti) 2011-06-23 07:25:28 PDT

Build id : Mozilla/5.0 (Android;Linux armv7l;rv:7.0a1)Gecko/20110623
Firefox/7.0a1 Fennec/7.0a1

Device: Motorola Droid 2
OS: Android 2.2
Comment 19 User image Dave Townsend [:mossop] 2011-08-01 20:46:38 PDT
*** Bug 675854 has been marked as a duplicate of this bug. ***
Comment 20 User image al_9x 2011-08-01 21:41:57 PDT
This does not work for unpacked extensions, which will be nearly all of them for older upgraded profiles (my case).  Copying such a profile changes the folder modified dates and the extensions remain disabled (Fx7).

Note You need to log in before you can comment on or make changes to this bug.