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

RESOLVED FIXED in Firefox 24

Status

RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
mozilla24
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:-, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: u=fx-os-user c=scravag-sprint p=1)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
asking tef? here per bug 860681 comment 28.
blocking-b2g: leo? → tef?
This is a dupe. Let me find it...
Keywords: qawanted
Whiteboard: DUPEME
Found it.
Status: NEW → RESOLVED
blocking-b2g: tef? → ---
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Whiteboard: DUPEME
Duplicate of bug: 822870
I'm closing out bug 822870 - it's a multi-definition bug.

Reopening this one to track fixing this for the homescreen only.
blocking-b2g: --- → tef?

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Updated

6 years ago
Blocks: 863032

Comment 5

6 years ago
Not user critical, not a blocker.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Keywords: regression
Not a regression - this has been known since the app updates implementation landed.
Keywords: regression
(Assignee)

Comment 7

6 years ago
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.
blocking-b2g: - → tef?
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?
Flags: needinfo?(felash)
(Assignee)

Comment 9

6 years ago
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.
Assignee: nobody → felash
blocking-b2g: tef? → leo?
Component: Gaia::Homescreen → DOM: Apps
Flags: needinfo?(felash)
Product: Boot2Gecko → Core
(Assignee)

Updated

6 years ago
Summary: [homescreen] if an icon is changed in an update, it's not updated before the next reboot → if an icon is changed in an update, it's not updated before the next reboot
(Assignee)

Comment 10

6 years ago
Created attachment 740250 [details] [diff] [review]
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.
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.
blocking-b2g: leo? → -
(Assignee)

Comment 12

6 years ago
Comment on attachment 740250 [details] [diff] [review]
patch v1

Seems to always work for me now, so r=fabrice?
Attachment #740250 - Attachment description: WIP patch v1 → patch v1
Attachment #740250 - Flags: review?(fabrice)
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 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.
Attachment #740250 - Flags: review?(fabrice) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 750977 [details] [diff] [review]
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.
Attachment #740250 - Attachment is obsolete: true
Attachment #750977 - Flags: review?(fabrice)
Julien - Can you get a test here?
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/
(Assignee)

Comment 18

6 years ago
Well yes, the second patch obsoletes the first one...

Ok, I'll add a test.
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?
Attachment #750977 - Flags: review?(fabrice) → review+
(Assignee)

Updated

6 years ago
Blocks: 874441
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1 → u=fx-os-user c=scravag-sprint p=1
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
(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/ ?
(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.
(Assignee)

Updated

6 years ago
Blocks: 876350
(Assignee)

Comment 23

6 years ago
Created attachment 754810 [details] [diff] [review]
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.
Attachment #750977 - Attachment is obsolete: true
Attachment #754810 - Flags: review?(ferjmoreno)
Julien - Do you have commit level 1? That's what needed to issue a try build.
(Assignee)

Comment 25

6 years ago
Jason, I should, I reopened Bug 853369 to see what happens.
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.
Attachment #754810 - Flags: review?(ferjmoreno) → review+
(Assignee)

Comment 28

6 years ago
Created attachment 755304 [details] [diff] [review]
patch v4

carrying r=fabrice,ferjm

fixed the test nits

thanks !
Attachment #754810 - Attachment is obsolete: true
Attachment #755304 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 29

6 years ago
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
Attachment #755304 - Flags: approval-mozilla-b2g18?
Attachment #755304 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/projects/birch/rev/7ee10964f9e7
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ee10964f9e7
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
(Assignee)

Comment 33

6 years ago
Filed bug 877630 for this.

Updated

5 years ago
No longer blocks: 863032

Updated

a year ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.