936018 - [Single Variant] Sometimes package apps icons are not correctly installed if finish quickly the FTE

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[rafael.marquez]

****Steps
1. Installing a v1.2 build on device
2. Clone the tests repository for sigle variant: https://github.com/telefonicaid/firefoxos-gaia-testsbuild.git
3. Configuring the file variant.json
4. Clone Gaia repository and select the master branch.
5. Executing the command "GAIA_DISTRIBUTION_DIR=/home/tel056/firefoxos-gaia-testsbuild  PRODUCTION=1 make reset-gaia" to install gaia with single variant in the device
6.Complete quickly the FTE  with a movistar SIM (in this case the configured in the file .json)

***Expected Result
Packaged apps are instaled correctly

***Actual Result
Packaged app are instaled with default icon

Device Unagi
Branch 1.2
Gecko 1303dc0
Gaia 2140c98

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Does rebooting the phone fix the problem (as in making the correct icon appear)? If it does, I think this is bug 936018.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #1)
> Does rebooting the phone fix the problem (as in making the correct icon
> appear)? If it does, I think this is bug 936018.

Ok, I meant bug 903291.

Comment 3

[Jason Smith [:jsmith]]

We probably need to fix this independently of bug 903291 - we have branding requirements that require the icon to be correct on preinstall.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #3)
> We probably need to fix this independently of bug 903291 - we have branding
> requirements that require the icon to be correct on preinstall.

The problem is that this *is* caused by bug 903291. And it doesn't actually depend on how fast you finish the FTE, rather it depends on how small the app is, and how unlucky you are. In fact, it can also happen with any app you install from any market, not just the single variant ones.

When the bug hits you, the application objects on the homescreen are created with an incorrect state, and that's what causes them to now show the correct icon (since homescreen believes they're still installing). The only solution (with the current implementation) is to either reboot the phone or to somehow kill the homescreen app. So a possible workaround would be killing/restarting the homescreen from system after the FTU is done (or just delaying creating it till then).

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

There's an ugly workaround for this, which is to kill the homescreen app after the operator apps have been installed. System should restart it automagically, and it would get the correct state for the apps. It's very ugly... but should work. 

Another ugly solution would be to make the app installation run slower. That would also give time to the homescreen to setup its handlers before the app is installed.

Other than that, I don't think this can be fixed completely without fixing bug 903291.

Comment 6

[Karen Ward [:kward]]

Changed to Koi+. This is a branding requirement and a Content Team requirement.

Comment 7

[Kai-Chih Hu [:khu]]

koi+ is for 1.2. We should use 1.2 target milestone rather than 1.3 target milestone.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #5)
> There's an ugly workaround for this, which is to kill the homescreen app
> after the operator apps have been installed. System should restart it
> automagically, and it would get the correct state for the apps. It's very
> ugly... but should work. 
> 
> Another ugly solution would be to make the app installation run slower. That
> would also give time to the homescreen to setup its handlers before the app
> is installed.
> 
> Other than that, I don't think this can be fixed completely without fixing
> bug 903291.

Bug 903291 isn't targeted for 1.2 given complexity. Let's go with the first proposed workaround, if only visible when you run through FTE too quickly (the issue this bug represents). If that's not possible, or the issue isn't FTE-specific, let's go with your second proposed workaround (a minimum app install time).

Can you move forward here amac?

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

I'm with this. But at this time I'm hitting into another problem, which I don't know if it's the original problem Rafa ran into. The apps aren't installed and I'm getting a SEC_ERROR_EXPIRED_CERTIFICATE when verifying the signature.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Ok, the SEC_ERROR_EXPIRED_CERTIFICATE happens because if the phone have been really reset it doesn't have the correct date, and so the apps aren't installed. I think that this might be the problem Rafa ran into. I'm going to upload a patch to not check the signature of single variant/local apps to fix that (because that's actually going to be a problem with real, mint-new devices).

Regarding the race condition, I haven't been able to reproduce it. In fact (and I stand corrected) the patch I made in https://bugzilla.mozilla.org/attachment.cgi?id=804889 should have taken care of most of the race condition issues (at least for successful installations, failures might run into race conditions still).

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

I uploaded a patch for the certificate problem at bug 926219, and I think I managed to reproduce this at least once... now I only have to find *why* it's still failing :)

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Gecko patch (B2G 26 version)

This just ensures that we actually update the object even when the process was already known to us. The previous version worked great on the tests, but not so great with real usage :(.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

There are actually two problems here. One with Gecko, which I think the patch I just uploaded fixes. Sometimes we weren't updating the state of the object on the child processes.

And the other problem is in Gaia, concretely in [1]. We're passing this as icon there, and we should really create a new object and reference the fields of this we actually need (uid and descriptor.icon only). Otherwise, we run into a nice problem: Assuming to updates arrive back to back, the first one starts processing but the second one doesn't since the validation it does at [2] is true always (basically because pendingRequest.icon and request.icon are exactly the same object). And that means the second request isn't processed and so the icon isn't updated.

I will upload a patch for Gaia tomorrow.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.js#L203
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/icon_retriever.js#L110

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8334913 [details] [diff] [review]
V1. Gecko patch (B2G 26 version)

This is the B2G26 version. Uploading the M-C version in a few (turns out this part have changed and it doesn't apply)

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Mozilla Central version

This is the same patch, but for the M-C repo. I don't really know if it's worth landing this on M-C now since I guess all this code is going away once Fernando finish rewriting all the Webapps related IPC, still this one is quite simple.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gaia patch to allow loading icons reentrantly

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8335212 [details] [diff] [review]
V1. Mozilla Central version

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

r=me with comment fixed.

::: dom/apps/src/Webapps.jsm
@@ +963,5 @@
>            refCount: 1
>          });
> +      }
> +
> +      // If it wasn't registered before, let's update its state

Update the comment.

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 8334913 [details] [diff] [review]
V1. Gecko patch (B2G 26 version)

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

r=me

::: dom/apps/src/Webapps.jsm
@@ +917,5 @@
>            refCount: 1
>          });
> +      }
> +
> +      // If it wasn't registered before, let's update its state

Update comment.

Comment 19

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8335422 [details] [review]
Gaia patch to allow loading icons reentrantly

Please request again a review when the unit tests are fixed and all comments are addressed what you consider needed or useful. Thanks

Comment 20

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8335422 [details] [review]
Gaia patch to allow loading icons reentrantly

Good job, thanks a lot

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2, Mozilla Central version with review comments addressed

r=fabrice

Try run at  https://tbpl.mozilla.org/?tree=Try&rev=bca7a595027d

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. B2G26_v_1_2 version, with review comments addressed

r=fabrice

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Gaia and Gecko patches are independent (so it doesn't matter the order they're checked in).

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

Gaia landed at: https://github.com/mozilla-b2g/gaia/commit/cdab4981044ddcf0ca32cab6e66a8141556701dc

Thanks Cristian!

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

The try run looks ok, requesting checkin for the gecko part. The Gaia patch already landed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4133b60c21ad

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4133b60c21ad

Comment 28

[Marcelino Veiga Tuimil [:sonmarce]]

Added flags to uplift patches to version 1.2

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/886a685e0d1e

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Sorry, forgot that there's a Gaia patch for this too. Setting status-b2g-v1.2 back to affected until that's uplifted.

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted cdab4981044ddcf0ca32cab6e66a8141556701dc to:
v1.2: 3c89aab6777f0ece3b30545b9b9bc7139c70d9b2

Comment 32

[rafael.marquez]

Verified:
Device unagi
Branch 1.2
Gecko 6ac8a1e
Gaia 264c604