923577 - Packaged role=system apps appear on homescreen after reboot

Status: VERIFIED FIXED

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

Description

[David Flanagan [:djf]]

When I install a packaged app with role=system (like the one at http://git.io/djf) it does not appear on the homescreen, as expected for role=system apps.  But after I reboot the phone, the app does appear on the homescreen.

When I tap the icon nothing is launched.  When I press and hold the icon we go into edit mode. I can then move the icon. But I cannot use the red x to uninstall it. (Using Settings->App Permissions does allow me to uninstall the app, though)

My packaged app does not have an icon, so I get the default rocket icon on the homescreen.

I suspect that this is closely related to bug 923574. It is the same packaged app used in both cases.

Steps to reproduce:

1) Follow the steps to reproduce bug 923574
2) Verify that there is no My Ringtones icon on the homescreen
3) Reboot
4) See that there is an icon on the homescreen

Comment 1

[David Flanagan [:djf]]

This might be another one for you to take or assign, Julien.

Comment 2

[Jason Smith [:jsmith]]

(In reply to David Flanagan [:djf] from comment #1)
> This might be another one for you to take or assign, Julien.

This sounds like a bug from the introduction of the role property in 1.2 with third-party keyboards. The system platform team would probably need to look into this.

Comment 3

[Jason Smith [:jsmith]]

If the intention of the role=system property is to never show in the UI, then this is a broken 1.2 feature if it shows up after reboot. 1.2 introduced this property through third-party keyboards, so right now, this is broken. Noming as such.

Comment 4

[Julien Wajsberg [:julienw]]

"system" role is definitely supposed to be hidden, see [1] and [2].

But probably you see it because :
* it's not installed because of bug 923574, and so you see the default icon to resume the download
* or app.manifest is null (and you might have an error in the logcat ?) because of a bug in the Webapps subsystem ?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L13
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L830-L833

Comment 5

[David Flanagan [:djf]]

(In reply to Julien Wajsberg [:julienw] (in MozSummit until next monday) from comment #4)
> "system" role is definitely supposed to be hidden, see [1] and [2].
> 
> But probably you see it because :
> * it's not installed because of bug 923574, and so you see the default icon
> to resume the download

You misunderstood that other bug report. The app installs correctly. It is just that the notification never goes away.

I then confused that other bug report by adding another comment about what seemed like a related issue in the case where the app does not install correctly.

But in this case, there the role=system app is installed correctly and works, but after a reboot, a non-functional icon shows up on the homescreen.

Comment 6

[Julien Wajsberg [:julienw]]

Sorry David, I was being unclear;

What I meant is that the root cause might be the same: the webapps subsystem and/or the homescreen might think the app is not installed yet. Or if app.manifest is undefined, just try to show at least something.

I'll investigate both next monday :)

Comment 7

[Julien Wajsberg [:julienw]]

Adds a dependency to bug 923574, we'll see if resolving that bug fix this one.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Not a System Platform bug.

Comment 9

[Gregor Wagner [:gwagner]]

David, (or maybe Jason) can we test if bug 923574 helped here?

Comment 10

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #9)
> David, (or maybe Jason) can we test if bug 923574 helped here?

I'm quite doubtful that it does - these two issues are not really related to each other. The patch included on bug 923574 involves validating the manifest size in the update manifest. This issue deals with what should or shouldn't be on the homescreen.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Gregor Wagner [:gwagner] from comment #9)
> > David, (or maybe Jason) can we test if bug 923574 helped here?
> 
> I'm quite doubtful that it does - these two issues are not really related to
> each other. The patch included on bug 923574 involves validating the
> manifest size in the update manifest. This issue deals with what should or
> shouldn't be on the homescreen.

Talked more with gerard-majax about this - it's probable because of the fallout that comes when bug 923574 reproduces, so it could be related. If someone doesn't get to this first, I'll look at this when I get the chance.

Comment 12

[:gerard-majax]

I'll look into this tomorrow :)

Comment 13

[:gerard-majax]

So, after bug 923574 landed, I've updated my Inari, installed the app from http://git.io/djf:
 - after install, not visible on homescreen
 - after reboot, not visible on homescreen
 - My Ringtones is proposed when changing ringtone.

Comment 14

[:gerard-majax]

Well for sure, the issue still reproduces, I probably got lucky earlier.

Comment 15

[:gerard-majax]

That is strange: it looks like if I remove the homescreen indexedDB's and stuff, stop and start b2g, the icon disappears for ever.

Comment 16

[:gerard-majax]

I think I'm getting a better picture now.

In apps/homescreen/js/grid.js: processApp(), when installing, I see:
 - app.manifest is undefined
 - app.updateManifest is defined

Hence, since manifest exists and it is updateManifest which does not contain the role, then the app gets displayed.

Comment 17

[:gerard-majax]

This manifest is being transmitted, http://davidflanagan.github.io/CustomFirefoxOSRingtones/myringtones.webapp.

This one does not contains the role=system. The manifest inside http://davidflanagan.github.io/CustomFirefoxOSRingtones/myringtones.zip does.

Comment 18

[:gerard-majax]

For now, as far as I could track it down, in Gecko (dom/apps/src/Webapps.js), the Gaia callback comes from the "Webapps:Install:Return:OK" case. We create the object at this time. When adding debug inside createApplicationObject() all I can see is that app.manifest is already undefined.

Comment 19

[:gerard-majax]

On Gecko side, when sending the Webapps:AddApp event, I have app:
{
   "installTime" : 1382539528952,
   "installerIsBrowser" : true,
   "manifestURL" : "http://davidflanagan.github.io/CustomFirefoxOSRingtones/myringtones.webapp",
   "csp" : "",
   "installerAppId" : 1005,
   "appStatus" : 1,
   "removable" : true,
   "storeVersion" : 0,
   "manifestHash" : "b018a9acc91875b8868e6a76f91b7800",
   "lastUpdateCheck" : 1382539528952,
   "installOrigin" : "http://davidflanagan.github.io",
   "installState" : "installed",
   "id" : "{e9256ef4-b5b9-45e9-aa2f-615b2e2ffb06}",
   "downloading" : false,
   "receipts" : [],
   "packageHash" : "9adc022146f5b539bba4e118b1556cb8",
   "progress" : 118318,
   "name" : "My Ringtones",
   "origin" : "app://{e9256ef4b5b9-45e9-aa2f-615b2e2ffb06}",
   "downloadSize" : 0,
   "readyToApplyDownload" : false,
   "basePath" : "/data/local/webapps",
   "storeId" : "",
   "etag" : null,
   "downloadAvailable" : false,
   "role" : "",
   "localId" : 1025
}

So the role is empty here ...

Comment 20

[Jason Smith [:jsmith]]

Sounds like this is a gecko bug then.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Alexandre LISSY :gerard-majax (off 16/09-14/10) from comment #16)
> I think I'm getting a better picture now.
> 
> In apps/homescreen/js/grid.js: processApp(), when installing, I see:
>  - app.manifest is undefined
>  - app.updateManifest is defined
> 
> Hence, since manifest exists and it is updateManifest which does not contain
> the role, then the app gets displayed.

I don't see a problem with this, while it's installing :)

Comment 22

[:gerard-majax]

So far, it looks like if I force the copy of role from aManifest to appObject, then the issue is not reproduced anymore. I did this in the callback of downloadPackage() that is available at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2178

Yet I can't tell if it's a legitimate fix; i.e. the original issue is that we are lacking this field, or if it's just a workaround that masks the real issue.

Fabrice, do you have any opinion on this ?

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

I didn't change anything in gecko when adding the role property, so if the case is in copying I'd say its a valid fix.

Comment 24

[[:fabrice] Fabrice Desré]

Alex, it's hard to say if you fix is ok without seeing the patch ;)

But in general, you have to wait for .manifest to be available, and not rely only on .updateManifest. What's in /data/local/webapps/webapps.json after the install is finished?

Comment 25

[:gerard-majax]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> Alex, it's hard to say if you fix is ok without seeing the patch ;)
> 
> But in general, you have to wait for .manifest to be available, and not rely
> only on .updateManifest. What's in /data/local/webapps/webapps.json after
> the install is finished?

After the installation, there is no role recorded in this file without the change I tested. If I apply my change, then the role is correctly set in this file.

Would it mean that my logic is the correct one ?

Comment 26

[:gerard-majax]

Here is the change I'm using and that works.

I've put it in the downloadPackage() method, after we receive and parse the real app manifest. I saw that before calling the aOnSuccess callback, we augment the 'app' object that is passed to downloadPackage() and that turns out to be the appObject variable which was missing the correct role.

As far as I could look into the code, I saw no tests for this.

diff --git a/dom/apps/src/Webapps.jsm b/dom/apps/src/Webapps.jsm
index 63cdba7..c96a73b 100644
--- a/dom/apps/src/Webapps.jsm
+++ b/dom/apps/src/Webapps.jsm
@@ -31,7 +31,7 @@ XPCOMUtils.defineLazyGetter(this, "libcutils", function() {
 #endif
 
 function debug(aMsg) {
-  //dump("-*-*- Webapps.jsm : " + aMsg + "\n");
+  dump("-*-*- Webapps.jsm : " + aMsg + "\n");
 }
 
 function supportUseCurrentProfile() {
@@ -2844,6 +2844,9 @@ this.DOMApplicationRegistry = {
                 }
                 app.appStatus = AppsUtils.getAppManifestStatus(manifest);
 
+                app.csp = manifest.csp || '';
+                app.role = manifest.role || '';
+
                 // Save the new Etag for the package.
                 if (aIsUpdate) {
                   if (!app.staged) {

Comment 27

[Julien Wajsberg [:julienw]]

Hey Fabrice, Alexandre,

The csp and role properties are not exposed to the content through the app object, but through the manifest property. Therefore the source of this bug is really in Homescreen (and Alexandre and I have a good lead right now :) ). So I'm moving component again.

I don't get why we copy the role and the csp properties to the appObject but I suppose you need it somewhere, and it's right that it doesn't seem to be copied correctly at install. Then it's always empty, which leads me to believe that either it's not really used and should be removed, or something else is broken elsewhere. Anyway I think this belongs to another bug. Alexandre, would you file a new bug for the gecko issue ? And we'll decide there whether we need to remove these properties or fix them.

Comment 28

[:gerard-majax]

Looks like julienw is right: the appObject lead was an erroneous one, and the issue really lies in the homescreen's grid code.

Comment 29

[[:fabrice] Fabrice Desré]

We added the role to the mozIApplication interface when working on 3rd party keyboards, because we thought that we needed it in gecko. I'll check if it's actually still the case (there was some back and forth on this) and decide whether to fix the install or to remove it.

Comment 30

[:gerard-majax]

Quick update: I have a fix, but it lack proper tests for now.

Comment 31

[:gerard-majax]

Attachment: bug923577.patch

Please find attached the tentative patch for this issue. Tests are not okay yet.

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 821944 [details] [diff] [review]
bug923577.patch

This is the Homescreen, let's ask feedback to Cristian ;)

Comment 33

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/13088

Please find attached a link to the github pull request that addreses this issue.

Comment 34

[Julien Wajsberg [:julienw]]

Comment on attachment 822175 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/13088

not much to say on the code, but I'd like that we try to use sinon's fake timers instead of setTimeout in the test code.

Comment 35

[Cristian Rodriguez (:crdlc)]

Comment on attachment 822175 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/13088

Great work! I've added a comment and please review Julien's suggestions. Thanks a lot

Comment 36

[:gerard-majax]

(In reply to Cristian Rodriguez (:crdlc) PTO (10-24 to 10-27) from comment #35)
> Comment on attachment 822175 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/13088
> 
> Great work! I've added a comment and please review Julien's suggestions.
> Thanks a lot

I've addressed both of your issues. I'm rechecking on Inari that everything is still fine. Tests are okay on my side.

Comment 37

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #36)
> (In reply to Cristian Rodriguez (:crdlc) PTO (10-24 to 10-27) from comment
> #35)
> > Comment on attachment 822175 [details] [review]
> > Link to Github https://github.com/mozilla-b2g/gaia/pull/13088
> > 
> > Great work! I've added a comment and please review Julien's suggestions.
> > Thanks a lot
> 
> I've addressed both of your issues. I'm rechecking on Inari that everything
> is still fine. Tests are okay on my side.

Everything seems fine on the device too. Let's wait for Travis ! In the mean time, if you have any other comment, feel free !

Comment 38

[Cristian Rodriguez (:crdlc)]

No, it is ok for me, thanks a lot!

Comment 39

[:gerard-majax]

Julien added some comments on tests, I've addres them, rechecked on device. I'm now waiting for a green travis to merge.

Comment 40

[:gerard-majax]

Travis is green, I'll merge when arriving home !

Comment 41

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/808aa2165bc2394f88b59a482e659609c513834d

Comment 42

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

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 808aa2165bc2394f88b59a482e659609c513834d
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 43

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/13167 for v1.2

Please find attached a link to the Github pull request for branch v1.2.

As far as I could test, it fixes the issue correctly on this branch. Please note, however, that unit tests are failing -- even before my patch -- on homescreen, hence no green travis :(

Comment 44

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

v1.2: 8a1c6a7

Comment 45

[Jason Smith [:jsmith]]

lgtm on trunk