863337 - if an icon is changed in an update, it's not updated before the next reboot

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* install an app whose you control the manifest
* change the icon in that manifest
* check for updates (the update will be automatically applied if the app is hosted without an appcache)

Expected:
* the new icon is there

Actual:
* the old icon is displayed until the next reboot

adding qawanted to see if that happens with other types of app too.

Comment 1

[Julien Wajsberg [:julienw]]

asking tef? here per bug 860681 comment 28.

Comment 2

[Jason Smith [:jsmith]]

This is a dupe. Let me find it...

Comment 3

[Jason Smith [:jsmith]]

Found it.

Comment 4

[Jason Smith [:jsmith]]

I'm closing out bug 822870 - it's a multi-definition bug.

Reopening this one to track fixing this for the homescreen only.

Comment 5

[Alex Keybl [:akeybl]]

Not user critical, not a blocker.

Comment 6

[Jason Smith [:jsmith]]

Not a regression - this has been known since the app updates implementation landed.

Comment 7

[Julien Wajsberg [:julienw]]

As I explained in bug 860681 comment 28, I'd like this bug to be tef+ instead of Bug 860681. That's why I nominated here.

Comment 8

[Wayne Chang [:wchang]]

Hi Julien,

How do you propose for this to be fixed and once that is done, how does it not cover the entirety of bug 860681?

Comment 9

[Julien Wajsberg [:julienw]]

So this is a gecko problem after all.

The main problem here (I think) is that we don't evict the manifest cache.

patch is coming.


What I don't understand yet is that when updating the apps, for one line of log in Webapps.jsm I get several line of logs in Webapps.js. Does that mean we keep some app objets around ? Fabrice would you know ?

Wayne> I ask for leo? here instead of tef? after all. I'll fix Bug 860681 correctly for tef.

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: patch v1

I _think_ I saw this working at one moment, but now it doesn't.

So this is a WIP for now as I have to work on other bugs first.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

The user isn't expecting an icon change, it does get picked up on reboot, this is not critical or a blocker so we'll leave it as tracking and please go ahead with uplift nomination if this is tested & low risk.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 740250 [details] [diff] [review]
patch v1

Seems to always work for me now, so r=fabrice?

Comment 13

[[:fabrice] Fabrice Desré]

Comment on attachment 740250 [details] [diff] [review]
patch v1

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

r=me with nits fixed

::: dom/apps/src/Webapps.js
@@ +375,5 @@
>  function WebappsApplication() {
>    this.wrappedJSObject = this;
>  }
>  
> +var hiddenProps = ['manifest', 'updateManifest'];

s/var/let, and use double quotes

@@ +376,5 @@
>    this.wrappedJSObject = this;
>  }
>  
> +var hiddenProps = ['manifest', 'updateManifest'];
> +var updatableProps = ['installOrigin', 'installTime', 'installState',

idem

@@ +379,5 @@
> +var hiddenProps = ['manifest', 'updateManifest'];
> +var updatableProps = ['installOrigin', 'installTime', 'installState',
> +    'lastUpdateCheck', 'updateTime', 'progress', 'downloadAvailable',
> +    'downloading', 'readyToApplyDownload', 'downloadSize'];
> +// props that we don't update: origin, receipts, manifestURL, removable

Please start the comment with a Capital and end with a full stop.

@@ +568,5 @@
> +        }, this);
> +
> +        hiddenProps.forEach(function(prop) {
> +          if (msg.app[prop]) {
> +            this['_' + prop] = msg.app[prop];

nit: double quotes in this file.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 740250 [details] [diff] [review]
patch v1

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

r=me with nits fixed

::: dom/apps/src/Webapps.js
@@ +375,5 @@
>  function WebappsApplication() {
>    this.wrappedJSObject = this;
>  }
>  
> +var hiddenProps = ['manifest', 'updateManifest'];

s/var/let, and use double quotes

@@ +376,5 @@
>    this.wrappedJSObject = this;
>  }
>  
> +var hiddenProps = ['manifest', 'updateManifest'];
> +var updatableProps = ['installOrigin', 'installTime', 'installState',

idem

@@ +379,5 @@
> +var hiddenProps = ['manifest', 'updateManifest'];
> +var updatableProps = ['installOrigin', 'installTime', 'installState',
> +    'lastUpdateCheck', 'updateTime', 'progress', 'downloadAvailable',
> +    'downloading', 'readyToApplyDownload', 'downloadSize'];
> +// props that we don't update: origin, receipts, manifestURL, removable

Please start the comment with a Capital and end with a full stop.

@@ +568,5 @@
> +        }, this);
> +
> +        hiddenProps.forEach(function(prop) {
> +          if (msg.app[prop]) {
> +            this['_' + prop] = msg.app[prop];

nit: double quotes in this file.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Finally found why it was not working sometimes: because for hosted app without appcache, we didn't change the app.updateTime property, and therefore homescreen skipped the icon retrieval.

So requesting review again for this change.

Comment 16

[Jason Smith [:jsmith]]

Julien - Can you get a test here?

Comment 17

[[:fabrice] Fabrice Desré]

Did you really mean to obsolete the first patch? Also, yes, adding a test would be nice. We already have hosted apps install/update tests in https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/

Comment 18

[Julien Wajsberg [:julienw]]

Well yes, the second patch obsoletes the first one...

Ok, I'll add a test.

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 750977 [details] [diff] [review]
patch v2

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

r=me with nits fixed.

::: dom/apps/src/Webapps.js
@@ +379,5 @@
> +let hiddenProps = ["manifest", "updateManifest"];
> +let updatableProps = ["installOrigin", "installTime", "installState",
> +    "lastUpdateCheck", "updateTime", "progress", "downloadAvailable",
> +    "downloading", "readyToApplyDownload", "downloadSize"];
> +// Props that we don't update: origin, receipts, manifestURL, removable.

Since you use these only in the Webapps:CheckForUpdate:Return:OK case, can you move them in the 'case' block?

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> Comment on attachment 750977 [details] [diff] [review]
> patch v2
> 
> Review of attachment 750977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits fixed.
> 
> ::: dom/apps/src/Webapps.js
> @@ +379,5 @@
> > +let hiddenProps = ["manifest", "updateManifest"];
> > +let updatableProps = ["installOrigin", "installTime", "installState",
> > +    "lastUpdateCheck", "updateTime", "progress", "downloadAvailable",
> > +    "downloading", "readyToApplyDownload", "downloadSize"];
> > +// Props that we don't update: origin, receipts, manifestURL, removable.
> 
> Since you use these only in the Webapps:CheckForUpdate:Return:OK case, can
> you move them in the 'case' block?

I was kind of hoping that we could reuse these vars for other cases too. But ok, I moved them.

Now I'm trying to write some tests.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> We already have hosted apps install/update tests in
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/

What about the tests in https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ ?

Comment 22

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> (In reply to Fabrice Desré [:fabrice] from comment #17)
> > We already have hosted apps install/update tests in
> > https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/
> 
> What about the tests in
> https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ ?

Most of those tests were originally targeted for desktop only (the mochitest chrome tests specifically). You'll probably want to build off the app update tests in the dom/apps/tests directory.

We do need to merge those two test directories into one, however.

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: patch v3

asking review from :ferjm for the tests part, as this is my first mochitest.

Plus my try access does not work so I can't trigger a try build.

Comment 24

[Jason Smith [:jsmith]]

Julien - Do you have commit level 1? That's what needed to issue a try build.

Comment 25

[Julien Wajsberg [:julienw]]

Jason, I should, I reopened Bug 853369 to see what happens.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Try build https://tbpl.mozilla.org/?tree=Try&rev=74220a0845cb

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 754810 [details] [diff] [review]
patch v3

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

Thanks Julien! Looks good. r=me

::: dom/apps/tests/file_packaged_app.template.webapp
@@ +8,5 @@
>    "developer": {
>      "name": "DEVELOPERTOKEN",
>      "url": "DEVELOPERURLTOKEN"
>    },
> +  "icons": {

No need to add this to the packaged app manifest template.

::: dom/apps/tests/test_app_update.html
@@ +233,5 @@
>          cb();
>        }
>      }
> +
> +    // this event is triggered when the app calls "alert"

nit: comments start with capital letter and ends with full stop.

Comment 28

[Julien Wajsberg [:julienw]]

Attachment: patch v4

carrying r=fabrice,ferjm

fixed the test nits

thanks !

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 755304 [details] [diff] [review]
patch v4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: Some properties of apps (especially the icons) are not updated before a reboot
Testing completed: yes + unit tests covering this case
Risk to taking this patch (and alternatives if risky): low 
String or UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/7ee10964f9e7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/09ac1fd2959c

Landed w/o the test changes per IRC conversation with Julien. He will file a follow-up for adding them on b2g18.

Comment 33

[Julien Wajsberg [:julienw]]

Filed bug 877630 for this.