Closed Bug 863337 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
mozilla24
blocking-b2g -
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: julienw, Assigned: julienw)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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.
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? → ---
Closed: 7 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?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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
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)
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
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
Attached patch patch v1 (obsolete) — Splinter Review
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? → -
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+
Attached patch patch v2 (obsolete) — Splinter Review
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/
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+
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
(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.
(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.
Blocks: 876350
Attached patch patch v3 (obsolete) — Splinter Review
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.
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+
Attached patch patch v4Splinter Review
carrying r=fabrice,ferjm

fixed the test nits

thanks !
Attachment #754810 - Attachment is obsolete: true
Attachment #755304 - Flags: review+
Keywords: checkin-needed
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/mozilla-central/rev/7ee10964f9e7
Status: REOPENED → RESOLVED
Closed: 7 years ago7 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.
Filed bug 877630 for this.
No longer blocks: b2g-apps-v1-next
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.