1058180 - Only create app migration observer component when absolutely needed

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Currently, we create an app migration component on profile-after-change, even in cases where we aren't in an upgrade path. We should be able to create one via a contract id only when upgrade is needed, saving us having the object possibly hanging around when not needed.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Only create app migrator when absolutely needed

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8489568 [details] [diff] [review]
Patch 1 (v1) - Only create app migrator when absolutely needed

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

::: dom/apps/Webapps.jsm
@@ +650,3 @@
>          // Run migration before uninstall of core apps happens.
> +        var appMigrator = Components.classes["@mozilla.org/app-migrator;1"]
> +                           .createInstance(Components.interfaces.nsIObserver);      

nit: trailing whitespace.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/6d28e030fd7b

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This is just a cleanup bug. Does not need aurora uplift.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/5f06a3b5fd42 due to causing desktop to crash. Whoops.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Backed out patch did not check for validity of requested appMigrator object. It just created it and tried to run it. Since migrators only exist on b2g, that was a bad idea. Retrying with new patch that actually does a validity check.

https://hg.mozilla.org/try/rev/20214cb35305

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #4)
> https://hg.mozilla.org/integration/b2g-inbound/rev/6d28e030fd7b

Note this also broke the webappstartup test on Autophone. Kyle, do you have a link to the try builds?

Comment 9

[Bob Clary [:bc] (inactive)]

see comment 8

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oh, yeah, sorry, pasted the wrong link in. Retried with a small change, still blew up everything.

https://tbpl.mozilla.org/?tree=Try&rev=e0206ab2d81d

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

New try, wrapping migration block in try/catch so we can just continue on errors.

https://tbpl.mozilla.org/?tree=Try&rev=c6ae131f903f

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Only create app migrator when absolutely needed

Just putting in for rereview to make sure fabrice is ok with the new error handling.

Comment 13

[Bob Clary [:bc] (inactive)]

Kyle, this new build still breaks webappstartup on at least Android 2.3 on a Samsung Galaxy GS 2. You can find the apk and instructions on how to run the test in bug 1062097.

Comment 14

[Bob Clary [:bc] (inactive)]

snorp, this is currently breaking webappstartup. Can you take a look at the patch?

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Bob Clary [:bc:] from comment #13)
> Kyle, this new build still breaks webappstartup on at least Android 2.3 on a
> Samsung Galaxy GS 2. You can find the apk and instructions on how to run the
> test in bug 1062097.

Which new build? The one in Comment 11?

Comment 16

[Bob Clary [:bc] (inactive)]

Kyle, yes that build but I just retested it and found that it worked fine. Not sure why I thought it failed this morning, but I made sure to reboot between steps this time and I'm not as tired.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/7e1f36eba077

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e1f36eba077