920481 - [app manager] if the app is running when we click "update", the app is not killed

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is stopping and starting the app after install what we want here?  Or is the some kind of faster "refresh" we can offer to reload the app?

Comment 3

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> Is stopping and starting the app after install what we want here?

Yes.

Comment 4

[Alexandre Poirot [:ochameau]]

That's a poor UX. Seeing the app close/open doesn't look very important/usefull and will take sigificant amount of time due to the animation. Also closing/opening apps are complex operations where we still don't really know exactly when the app is done closing/opening. So I'm expecting such behavior would be harder to implement right.
I think we should try to do what simulator was doing, i.e. refreshing the frame.
The simulator was having the bad practice to call gaia codebase, it was calling this method:
  https://mxr.mozilla.org/gaia/source/apps/system/js/window.js#143
But we can simply fetch the frame from the actor and call frame.reload(true).

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Reload app frame after update

Alex, I've attempted your plan of calling frame.reload(true), but it doesn't seem to work too well for me.  The page flashes briefly while reloading, but it reappears with the old content.  Is there some additional cache clearing I need to do?

I am able to see the new content by stopping and starting the app after install.

Comment 6

[Alexandre Poirot [:ochameau]]

I've been looking at this and for me it seems to work only once, that's a really weird behavior.
Luca identified that we are missing a fix we had in the old simulator app actor, where we were flushing the jar:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1408

I tried to also fire this flush notification, but it doesn't fix this...

I'll look into this.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Call BrowserTabActor.reload when the app is updated to refresh the app r=paul

Here is the UI patch, that just call the existing reload method of BrowserTabActor that does a window.location.reload()

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Ensure flushing jar cache when updating an app r=paul

Here is a device fix, that's an uplift from the old simulator codebase.
We need to flush the jar cache when updating the app zip package.
But it is a bit more complex on device, with apps running OOP.
We have to flush the cache in the app process itself.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 818574 [details] [diff] [review]
Call BrowserTabActor.reload when the app is updated to refresh the app r=paul

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

::: browser/devtools/app-manager/content/projects.js
@@ +187,5 @@
> +        });
> +  },
> +
> +  reload: function (project) {
> +    getTargetForApp(this.connection.client,

This whole function could be moved to app-actor-front.js or a similar file.  I was liking the trend of separating UI logic from the calls to the debugger API that we got from having things in app-actor-front.js, so it would be nice to continue that.

That could return a promise you can use to log "App reloaded".

Hopefully "soon" we can refactor this file a bit, as it is quite a mess...

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 818576 [details] [diff] [review]
Ensure flushing jar cache when updating an app r=paul

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

Looks good overall!  It's quite a nice UX to see it reload in place!  :D

::: toolkit/devtools/server/actors/webapps.js
@@ +422,5 @@
> +          // http://hg.mozilla.org/mozilla-central/annotate/aaefec5d34f8/dom/apps/src/Webapps.jsm#l1125
> +          // Do it in parent process for the simulator
> +          let jar = installDir.clone();
> +          jar.append("application.zip");
> +          Services.obs.notifyObservers(jar, "flush-cache-entry", null);

Is there any harm to running this on device?  I guess it will just be ignored safely...?

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: UI patch, added a reloadApp helper in app-actor-front r=paul

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> ::: toolkit/devtools/server/actors/webapps.js
> @@ +422,5 @@
> > +          // http://hg.mozilla.org/mozilla-central/annotate/aaefec5d34f8/dom/apps/src/Webapps.jsm#l1125
> > +          // Do it in parent process for the simulator
> > +          let jar = installDir.clone();
> > +          jar.append("application.zip");
> > +          Services.obs.notifyObservers(jar, "flush-cache-entry", null);
> 
> Is there any harm to running this on device?  I guess it will just be
> ignored safely...?

Yes, especially since we give the precise application jar path. The jar channel will only prune cache for this particular jar.

Comment 13

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/54da01870611
https://hg.mozilla.org/integration/fx-team/rev/1864e27d5ee4

Comment 14

[Ed Morley [:emorley]]

Backed out for test_webapps_actor.html timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29550901&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=29551027&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/dad5e53c31a9
remote:   https://hg.mozilla.org/integration/fx-team/rev/987ac1967cbe

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Ensure flushing jar cache when updating an app. [fixed desktop exception]

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 821060 [details] [diff] [review]
Ensure flushing jar cache when updating an app. [fixed desktop exception]

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

::: toolkit/devtools/server/actors/webapps.js
@@ +768,5 @@
>    _appFrames: function () {
> +    // For now, we only support app frames on b2g
> +    if (Services.appinfo.ID != "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}") {
> +      return;
> +    }

I had to add this to prevent test exceptions on desktop.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 821060 [details] [diff] [review]
Ensure flushing jar cache when updating an app. [fixed desktop exception]

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

Looks good.  Would not have guessed that was needed, but good to know for the future!

Comment 18

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=3004cda61bb9

Comment 19

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/8bc8a56998dd
https://hg.mozilla.org/integration/fx-team/rev/874714520261

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bc8a56998dd
https://hg.mozilla.org/mozilla-central/rev/874714520261

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 821060 [details] [diff] [review]
Ensure flushing jar cache when updating an app. [fixed desktop exception]

NOTE: This flag is now for security issues only. 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 #): new feature (app manager)
User impact if declined: repushing an app via the app manager doesn't update the app, the user has to kill the app manually
Testing completed: works with FxOS 1.3
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 22

[Paul Rouget [:paul]]

Alex, can we uplift that to the Simulator in the meantime?

Comment 23

[Garrett Robinson [:grobinson]]

I have this problem with the 1.2 simulator and it is quite annoying. Please uplift ASAP?

Comment 24

[Alexandre Poirot [:ochameau]]

We no longer build a custom b2g runtime, nor have a custom copy of webapps actor.
So we can't really uplift it as-is on the simulator without uplifting this patch on m-c.

Otherwise, we should ensure that approval-mozilla-b2g26 is still tracked...

Comment 25

[Paul Rouget [:paul]]

(In reply to Garrett Robinson [:grobinson] from comment #23)
> I have this problem with the 1.2 simulator and it is quite annoying. Please
> uplift ASAP?

Can you use version 1.3 in the meantime?

Comment 27

[Jason Smith [:jsmith]]

Go ahead & land this - this is critical for dev tools experience with apps.

Comment 28

[Paul Rouget [:paul]]

(In reply to Jason Smith [:jsmith] from comment #27)
> Go ahead & land this - this is critical for dev tools experience with apps.

I'm not sure how to land this. What's the repo?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7224ee49d76a
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7d6aae88ff50

Comment 30

[Garrett Robinson [:grobinson]]

This is awesome! Just downloaded the latest 1.3 simulator and clicking "Update" worked perfectly. Thanks everyone! (Now I can continue working on adding OTR support to https://github.com/waalt/loqui with minimal frustration)

Comment 31

[Petruta Horea [:phorea]]

Verified as fixed on Firefox 28 beta 6 under Win 7 64-bit and Ubuntu 13.04 64-bit using 1.3 Simulator and 
an inagi device having:
Gaia      49312fb89b90eca48a8011b94a1a1a751daf32d2
Gecko     https://hg.mozilla.org/mozilla-central/rev/1507f021ac93
BuildID   20140225160202
Version   30.0a1
ro.build.version.incremental=eng.cltbld.20140225.192300
ro.build.date=Tue Feb 25 19:48:26 EST 2014