Last Comment Bug 710961 - add-on syncGUID being set twice
: add-on syncGUID being set twice
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Gregory Szorc [:gps]
:
Mentors:
Depends on: 534956 704973
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 21:06 PST by Gregory Szorc [:gps]
Modified: 2012-01-25 08:14 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Reuse syncGUID, v1 (5.73 KB, patch)
2011-12-20 16:24 PST, Gregory Szorc [:gps]
dtownsend: review+
Details | Diff | Splinter Review
Reuse syncGUID, v2 (5.95 KB, patch)
2011-12-30 14:46 PST, Gregory Szorc [:gps]
gps: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2011-12-14 21:06:47 PST
I noticed that it is possible for a syncGUID to get generated twice during some install scenarios.

I /think/ it reproduces for non-restartless add-ons only:

1) Install add-on from AddonInstall. In the onInstallEnded, the addon.syncGUID is defined as one thing.
2) Restart the application
3) The add-on actually gets installed and syncGUID is regenerated.

I would expect the syncGUID from the first run to get carried over.

This isn't a deal-breaker: Sync will reconcile the different syncGUIDs as things are synchronized. But, it is annoying.

I'll address this as a follow-up to add-on sync.
Comment 1 Dave Townsend [:mossop] 2011-12-20 15:42:16 PST
I think you just need to add syncGUID to here http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#7277
Comment 2 Gregory Szorc [:gps] 2011-12-20 16:24:02 PST
Created attachment 583334 [details] [diff] [review]
Reuse syncGUID, v1

Adding the syncGUID field to importMetadata seemed to do it!

I verified the new test code in test_install.js failed without the change to XPIProvider.jsm applied. test_bootstrap.js worked both before and after (I think that makes sense).

My biggest concern with this patch is that I don't have enough test coverage. Is there anywhere else I really need test coverage?
Comment 3 Dave Townsend [:mossop] 2011-12-21 10:13:41 PST
Comment on attachment 583334 [details] [diff] [review]
Reuse syncGUID, v1

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

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
@@ +148,5 @@
>      do_check_eq(install.type, "extension");
>      do_check_eq(install.version, "1.0");
>      do_check_eq(install.name, "Test Bootstrap 1");
>      do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
> +    do_check_neq(install.addon.syncGUID);

do_check_neq expects two arguments, I guess this is just checking it isn't undefined but be explicit about it

@@ +189,5 @@
>  
>      AddonManager.getAddonByID("bootstrap1@tests.mozilla.org", function(b1) {
>        do_check_neq(b1, null);
>        do_check_eq(b1.version, "1.0");
> +      do_check_neq(b1.syncGUID);

ditto
Comment 4 Gregory Szorc [:gps] 2011-12-21 11:32:43 PST
Pushed to inbound with extra null argument to do_check_eq() added: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d2a543a0d1
Comment 5 Ed Morley [:emorley] 2011-12-22 03:51:22 PST
https://hg.mozilla.org/mozilla-central/rev/f8d2a543a0d1
Comment 6 Gregory Szorc [:gps] 2011-12-30 14:46:59 PST
Created attachment 585054 [details] [diff] [review]
Reuse syncGUID, v2

This is the actual patch that was committed. Adding as attachment because I'm requesting approval for Aurora. I would like this merged to Aurora because it improves the add-on sync experience and reduces the potential for bugs due to bug 710448.
Comment 7 Alex Keybl [:akeybl] 2012-01-04 17:42:12 PST
Comment on attachment 585054 [details] [diff] [review]
Reuse syncGUID, v2

[Triage Comment]
Because of where we are in the cycle, and the possibility for data loss as outlined in 710448, I'm approving for Aurora.
Comment 8 Gregory Szorc [:gps] 2012-01-04 18:13:56 PST
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/e03d758c3c08
Comment 9 Tracy Walker [:tracy] 2012-01-18 08:48:27 PST
gps: is there anything to verify here?
Comment 10 Gregory Szorc [:gps] 2012-01-19 11:37:22 PST
Tracy: This was really an internal optimization. I'm not sure there is an easy way to verify this beyond verifying the Add-on Manager xpcshell tests pass.

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