892708 - [B2G] Unable to switch back and forth between apps

Status: RESOLVED FIXED

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

Description

[dkumar]

Description:
User is not able to access the camera app after tapping on the camera icon from the gallery app, and is also not able to access gallery from the camera app.

Repro Steps:
1. Updated to Build ID: 20130711070209.
2. Open the Gallery app
3. Click on the camera icon
4. Exit out the application and click on the camera app
5. Try taping on the gallery view

Actual:
When user is in Gallery app, Camera cannot be accessed. 
When user is in Camera app, Gallery view cannot be accessed. 

Expected:
The Camera app is opened and ready to take a picture from the gallery app.
Gallery is open to select the pictures from the camera app.

Environmental Variables
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/aaa74e5efaf1
Gaia: 7cdcc46179d198cab11970964b181ede32a5b683
Platform Version: 18.0

Notes: reproduces on leo and unagi devices
Repro frequency: 100%

Comment 1

[dkumar]

Video: http://youtu.be/GbdtiAFmeYQ

Comment 2

[Marcia Knous [:marcia]]

I see this on Leo using a slightly earlier build:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/b64372810dd7
Gaia   680c72ccbd2d5045e590a6ba08de2aac6af71953
BuildID 20130710230201
Version 18.1

I assume this would have been caught in the smoketest yesterday, so I can flash back to that build and see.

Comment 3

[Marcia Knous [:marcia]]

Attachment: Logcat captured during testing

Here is the regression range using Leo device:

Works:
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635
Gaia   61453c4d32beb15a33ec91b2e740e96e5ce45759
BuildID 20130710070204
Version 18.1

Broken:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/b64372810dd7
Gaia   680c72ccbd2d5045e590a6ba08de2aac6af71953
BuildID 20130710190458
Version 18.1

Comment 5

[Jason Smith [:jsmith]]

I'm pretty sure this is a regression from bug 853759.

Comment 6

[[:fabrice] Fabrice Desré]

fwiw, I can't repro on a gecko-trunk + gaia-master build.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> fwiw, I can't repro on a gecko-trunk + gaia-master build.

What about b2g18?

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Fabrice Desré [:fabrice] from comment #6)
> > fwiw, I can't repro on a gecko-trunk + gaia-master build.
> 
> What about b2g18?

Actually, looks like I can't repro either. The dupe and this bug work fine for me on a 7/12 build.

QA Wanted for a retest.

Comment 9

[Natalya Kot [:nkot]]

it looks like it only works for the first time after launching the app 

 - able to switch to Camera from Gallery (or vice versa) but unable to switch it back,    
 - able to send SMS from Contacts for the first time, need to kill the app in order to send another SMS from Contacts

Leo
Build  20130712070210
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988

Comment 10

[Jason Smith [:jsmith]]

Okay, comment 9's STR does reproduce the bug for me.

Example STR

1. Open the Contacts app
2. Create two contacts with two phone numbers
3. Select the first contact and select the SMS symbol
4. Go back to the Contacts app
5. Select the second contact and select the SMS symbol

Result - step #3 will move the user to the SMS app, but step #6 will not move the user to the SMS app.

Comment 12

[Jason Smith [:jsmith]]

Attachment: Send the open-app message always for activity type system messages

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 775641 [details] [diff] [review]
Send the open-app message always for activity type system messages

This is the patch that was proposed for bug 893703.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

I did test the patch also on the other STR (well, in fact I had tested it before finding this bug :) ) and it works also. The patch just leaves activities working as before.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking since this is a smoketest regression.

Comment 17

[Allen Maxwell]

From bug 892573:
Contact SMS button responds to touch, but SMS will not initiate

Environmental  Variables:
Build ID: 20130715070218
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8
Gaia: 55ed5e08a2250ea2d3571fff860c39e66fabed14
Platform Version: 18.1
RIL Version: 01.01.00.019.158

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment on attachment 775641 [details] [diff] [review]
Send the open-app message always for activity type system messages

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

I'm not sure if Fabrice would like this or not. He used to mention the system mechanism shouldn't be aware of the system message type to have specific logic. However, I'm kind of fine with that if Fabrice doesn't have strong opinion. Maybe, a better solution is like what I've mentioned at bug 888926, comment #12.

  - When the app is not running, fire 'open-app'.
  - When the app is still running, fire 'load-page' (or a better naming).

And the window_manager.js can check if it satisfies the conditions of 'load-page' and 'activity' to do the special purpose for activities. This solution needs both changes on the Gecko and Gaia.

::: dom/messages/SystemMessageInternal.js
@@ +180,5 @@
>      if (page) {
>        // Queue this message in the corresponding pages.
>        this._queueMessage(page, aMessage, messageID);
>  
> +        if ((atype === "activity") || (result === MSG_SENT_FAILURE_APP_NOT_RUNNING)) {

nit: please align this if-block.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

I'm not very fond of this solution myself, but it's the easiest, fastest, and less riskier one. As for not having the system mechanism aware of the frontend logic, the problem is that they *have* different semantics and different syntax. Having the backend unaware of this just causes problems down the road.

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Add a 'must open' type of messages

This is basically the same solution, slighty cleaner and clearer.
If you prefer me to add a load-app event that gets send when the message has already been sent I can do that instead, but at this step that introduces more noise.

Note that, as I mentioned previously, if the code is higly coupled (as it happens currently with this code and windows_manager.js) I prefer it to be evident at both sides of the coupling... that way at least you're aware of the side effects of any change just by looking at any part of the code. That said, it's your call.

So as to no lose more time, I'll go ahead and start on the other version too, that way you can choose whichever you like more.

Comment 21

[[:fabrice] Fabrice Desré]

I really don't want to see some system message names being special cased in the system messages code directly. The dependency is clearly that Activities are built on top of system messages, so if we need something to make them work we have to:
- make sure it's really activity specific. I don't think we put too much though into that here... won't the push api have the same needs?
- if there is some specific behavior for a message type, it should be pluggable. This is what we did for the object wrapping at the chrome -> content boundary: by default we just use ObjectWrapper, but if a custom wrapper is provided for a message type, we switch to this wrapper. See https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageManager.js#82 and https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivityWrapper.js

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> I really don't want to see some system message names being special cased in
> the system messages code directly. The dependency is clearly that Activities
> are built on top of system messages, so if we need something to make them
> work we have to:
> - make sure it's really activity specific. I don't think we put too much

> though into that here... won't the push api have the same needs?

Actually no... the push API doesn't do any magic with the System Messages, it uses them explictly (not implicitly like activities) and don't have any 'special treatment' by the windows manager either. Is the app receiving the push message the one that decides what to do with it. The only danger to the push API would be reverting the patch that caused the problem for activities, because then any handler that's not defined on index.html and that has any kind of asynchronous processing (like, say, downloading the actual data from the app server) will fail halfway. 


> - if there is some specific behavior for a message type, it should be
> pluggable. This is what we did for the object wrapping at the chrome ->
> content boundary: by default we just use ObjectWrapper, but if a custom
> wrapper is provided for a message type, we switch to this wrapper. See
> https://mxr.mozilla.org/mozilla-central/source/dom/messages/
> SystemMessageManager.js#82 and
> https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/
> ActivityWrapper.js

Ok, do you prefer for me to implement a pluggable message treatment in Gecko for activity messages and leave Gaia alone, or just to implement Alive's suggestion and sending a different message to window manager when it has to actually open the app, and when it just has to load it (and leave the activity specific code at window_manager.js, which already has specific code)?

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Note that activities are also susceptible to the original problem. They way it's running now is that if the handler page is loaded then the Activity handler is invoked (the message is delivered and marked as delivered) and *then* potentially the page can be reloaded while the handler is running.

I think the best way to mitigate that is the second option (Gecko + Gaia change):

Gecko: change it so when the message is already been delivered a different message is passed to Gaia (load-app instead of open-app?)

Gaia: leave on load-app only the activity "magic" to make the receiving app appear on top. Don't reload the pages (cause that might break the handlers), just shuffle them,


Although the easiest change is the Gecko only one, implement a pluggable 'decidor' that decides if the open-app should be passed for a given activity or not.

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

On second thought, there's a third solution which I think it's better:

* Implement a pluggable deciding module to decide whether a message is safe to send if the page is already loaded.
* On _sendCommon call the specificMessageHandler.safeToSend(whatever) (names are a WIP :P ) to decide if the message can be send. If safeToSend returns false then just queue it and return MSG_SENT_FAILURE_APP_NOT_RUNNING.
* Leave the rest of the code as is.

This way, for normal messages the message will be sent as it's sent now, but for Activities we can return false and then the message will be sent *after* window_manager.js opens the app (which makes more sense than how's it working now, anyway).

I'm going to implement this one, if it's ok with you.

Comment 25

[[:fabrice] Fabrice Desré]

That looks fine to me.

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Add a plugable way to selectively block sending the message before opening the window

This implements what was described at comment 24.

The patch doesn't apply cleanly to B2G18, will do the patch for that version once the review is done.

Tried it with the camera/gallery, with contacts/sms and contacts/gallery, and used a push enabled app for the normal messages flow.

Comment 27

[Chris Kreinbring [:CKreinbring]]

I noticed that the bug only occurs if the app to be opened is already open.  For example, tapping the gallery icon while in the Camera app will launch the gallery if it is not already open.

Comment 28

[Antonio Manuel Amaya Calvo (:amac)]

Yeah, that's the root of the problem, the change I made to avoid apps being reloaded after the message has been delivered broke activities cause activities rely on being always reloaded/reopened/reshuffled. The proposed patch fixes that.

Comment 30

[Gene Lian (I already quit Mozilla)]

Comment on attachment 776468 [details] [diff] [review]
Add a plugable way to selectively block sending the message before opening the window

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

I'm worried about this design. What's happening if the app has already opened to call mozSetMessageHandler("activity", ...) once and is still alive at the top of the screen? Can the 'open-app' trigger the app to call it again in any way?

::: dom/activities/src/ActivityMessageConfigurator.js
@@ +13,5 @@
> +  // dump("-- ActivityMessageConfigurator.js " + Date.now() + " : " + aMsg + "\n");
> +}
> +
> +/**
> +  * nsISystemMessagesConfigurator implementation. 

nit: TS.

::: dom/messages/SystemMessageInternal.js
@@ +47,5 @@
>    // dump("-- SystemMessageInternal " + Date.now() + " : " + aMsg + "\n");
>  }
>  
>  
> +var defaultMessageConfigurator = {

nit: s/var/let/

@@ +51,5 @@
> +var defaultMessageConfigurator = {
> +  safeToSendBeforeFE: function safeToSendBeforeFE() {
> +    return true;
> +  }
> +}

nit: add an ";" after each JS statement.

@@ +91,5 @@
>  
>  SystemMessageInternal.prototype = {
>  
> +  _getMessageConfigurator: function _getMessageConfigurator(aType) {
> +    debug("_getMessageConfigurator");

debug("_getMessageConfigurator for type: " + aType);

@@ +96,5 @@
> +    if (this._configurators[aType] === undefined) {
> +      let contractID = "@mozilla.org/dom/system-messages/configurator/" + aType + ";1";
> +      if (contractID in Cc) {
> +        debug(contractID + " is registered, creating an instance");
> +        this._configurators[aType] = Cc[contractID].createInstance(Ci.nsISystemMessagesConfigurator);

nit: too long. Please fold this to be:

this._configurators[aType] =
  Cc[contractID].createInstance(Ci.nsISystemMessagesConfigurator);

@@ +574,2 @@
>  
> +    if (this._getMessageConfigurator(aType).safeToSendBeforeFE()) {

Could you please add some comments before this condition to describe why you need to do this check?

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +56,5 @@
> +interface nsISystemMessagesConfigurator: nsISupports
> +{
> +  /*
> +   * Returns true if this type of system messages is safe to send
> +   * before the frontend has processed it.

s/processed it/activated the app to process it/

@@ +58,5 @@
> +  /*
> +   * Returns true if this type of system messages is safe to send
> +   * before the frontend has processed it.
> +   */
> +  bool safeToSendBeforeFE();

What does FE stand for? Front-end? It sounds unclear to me to describe the purpose of this function. I'd suggest renaming this to:

safeToSendBeforeActivatingApp or
safeToSendBeforeRunningApp

Or other better names if you'd have one. I originally thought safeToSendBeforeOpeningApp but the app might be actually opened.

I think safeToSendBeforeRunningApp might be better. First of all, it's shorter; second, it also makes sense to the original enumeration name MSG_SENT_FAILURE_APP_NOT_RUNNING.

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review Gene. 

The only way for an app to be alive and on top when it receives an activity is if it sent the activity to itself. And I think in that case it'll be reloaded again (on the original code path also). The only thing the patch does is to modify the order (open first, deliver the message second). 

I'll made the modifications you requested after Fabrice's review. What does TS mean, BTW?

Comment 32

[Gene Lian (I already quit Mozilla)]

(In reply to Antonio Manuel Amaya Calvo from comment #31)
> The only way for an app to be alive and on top when it receives an activity
> is if it sent the activity to itself. And I think in that case it'll be
> reloaded again (on the original code path also). The only thing the patch
> does is to modify the order (open first, deliver the message second). 

It's exactly what I thought, but [1] doesn't reload the page. Right? Did I misunderstand something?

Also, please be careful to verify the code path at [2] for opening an in-line window. That is, for an app A that is already opened [3], the order of opening app A's in-line window from app B and then sending an activity message to it is still working.

[1] https://github.com/mozilla-b2g/gaia/blob/10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.js#L1330

[2] https://github.com/mozilla-b2g/gaia/blob/10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.js#L1312

[3] Is it possible to open app A's in-line window as well in advance? That's what I'm really concerned.

> 
> I'll made the modifications you requested after Fabrice's review. What does
> TS mean, BTW?

Trailing Space. :)

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Gene Lian [:gene] from comment #32)
> (In reply to Antonio Manuel Amaya Calvo from comment #31)
> > The only way for an app to be alive and on top when it receives an activity
> > is if it sent the activity to itself. And I think in that case it'll be
> > reloaded again (on the original code path also). The only thing the patch
> > does is to modify the order (open first, deliver the message second). 
> 
> It's exactly what I thought, but [1] doesn't reload the page. Right? Did I
> misunderstand something?

No... but I don't think sending activities to yourself (not inline) works that well in any case, since if the treating page isn't the same than the originating one then the originating one will be replaced with the treating one (and there goes your onsuccess, onerror or whatever). The only way it might work (with the original code even) is if both the originating page and the treating page are the same, and then the activity is just a (very) convoluted way of invoking a function on the same page. And I don't know what the semantic would be expected to be in that case... (does the handler method gets invoked after the calling one ends? what happens if it doesn't end? Is it reentered?...)

In any case, that's a fringe case that could be fixed at Gaia (as part of the setDisplayedApp that gets invoked for activities always).

For our cases, I did test with (both with apps running and dead): 
 
* Gallery->Camera->Gallery 
* Camera->Gallery->Camera
* Contacts->Gallery->Contacts (inline, pick contact foto)
* Contacts->SMS (Send sms)
* SMS->Contacts (inline, pick contact)
* Widget->Cost Control

and it works correctly on all of them. I can make a test app that sends activities to itself (since I don't think we currently have any that do that) and see what happens :)

> 
> Also, please be careful to verify the code path at [2] for opening an
> in-line window. That is, for an app A that is already opened [3], the order
> of opening app A's in-line window from app B and then sending an activity
> message to it is still working.

That I've tried, with the contacts app and the gallery (when you use the gallery to choose a photo). Tested it both with the gallery previously loaded and with the gallery not loaded. As a matter of fact it should work better now since before we could/would deliver the message to the pre-opened app and then load the dialog on a different frame (that's what inlining does). With this change the newly opened dialog should get the message instead.


> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.
> js#L1330
> 
> [2]
> https://github.com/mozilla-b2g/gaia/blob/
> 10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.
> js#L1312
> 
> [3] Is it possible to open app A's in-line window as well in advance? That's
> what I'm really concerned.
> 
> > 
> > I'll made the modifications you requested after Fabrice's review. What does
> > TS mean, BTW?
> 
> Trailing Space. :)

Thanks :)

Comment 34

[Gene Lian (I already quit Mozilla)]

Comment on attachment 776468 [details] [diff] [review]
Add a plugable way to selectively block sending the message before opening the window

Thanks for the detailed verifications! We still need Fabrice's double review before landing.

Comment 35

[[:fabrice] Fabrice Desré]

Comment on attachment 776468 [details] [diff] [review]
Add a plugable way to selectively block sending the message before opening the window

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

r=me with comments addressed.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +56,5 @@
> +interface nsISystemMessagesConfigurator: nsISupports
> +{
> +  /*
> +   * Returns true if this type of system messages is safe to send
> +   * before the frontend has processed it.

Also add what the default value is when this is not provided for a message type.

@@ +58,5 @@
> +  /*
> +   * Returns true if this type of system messages is safe to send
> +   * before the frontend has processed it.
> +   */
> +  bool safeToSendBeforeFE();

There's no need for that to be a function. I think I prefer a readonly boolean property.

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Add a plugable way to selectively block sending the message before opening the window, V2

Keeping the r+ from the previous version, r=fabrice, r=gene

This just adds the changes requested by the reviewers comments.
The patch does not apply cleanly on B2G18, will upload a specific version for that.

Comment 37

[[:fabrice] Fabrice Desré]

Comment on attachment 777087 [details] [diff] [review]
Add a plugable way to selectively block sending the message before opening the window, V2

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

::: dom/messages/SystemMessageInternal.js
@@ +96,5 @@
> +    if (this._configurators[aType] === undefined) {
> +      let contractID = "@mozilla.org/dom/system-messages/configurator/" + aType + ";1";
> +      if (contractID in Cc) {
> +        debug(contractID + " is registered, creating an instance");
> +        this._configurators[aType] = 

nit: trailing whitespace.

Comment 38

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Add a plugable way to selectively block sending the message before opening the window, V2.1

Keeping r+ from the previous version r=fabrice, r=gene

And fixed the nit TS :)

Will upload a B2G18 version now, since this doesn't apply cleanly

Comment 39

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Add a plugable way to selectively block sending the message before opening the window, B2G18 version

r=fabrice, r=gene

This is the B2G18 version, the only change is modifying the Makefile.in instead of moz.build.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e67c8f1fcab2

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e67c8f1fcab2

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a0b11d87b196

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a0b11d87b196

Comment 46

[J Zimbrick]

Issue no longer appears to be occurring in the latest Leo build from the commercial 1.1 branch: 

Environmental  Variables:
Build ID: 20130718070206
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/7f6f4bc1a621
Gaia: f1d2e3fd806dc55f167c72ac8ef7a3b6baed915e
Platform Version: 18.1
RIL Version: 01.01.00.019.164

I'm able to switch back and forth between the camera and gallery using the button in each app, as well as able to send SMS from a contact detail.

Comment 47

[J Zimbrick]

Same results as Comment 46 on an Unagi device running the latest build from the mozilla 1.2 branch:

Environmental  Variables:
Build ID: 20130718030209
Gecko: http://hg.mozilla.org/mozilla-central/rev/f26e4c26ce4a
Gaia: 4ec7c428f6a63a44f888ea6f6ade0385c89ae305
Platform Version: 25.0a1

App switching seems to be working as intended.

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

Concerning the LeoVB-, to be able to land this one, you need to land bug 888926 previously.

Comment 49

[Gene Lian (I already quit Mozilla)]

This is causing another regression for activity at Bug 896945. :(

Comment 50

[Gene Lian (I already quit Mozilla)]

(In reply to Antonio Manuel Amaya Calvo from comment #33)
> For our cases, I did test with (both with apps running and dead): 
>  
> * Gallery->Camera->Gallery 
> * Camera->Gallery->Camera
> * Contacts->Gallery->Contacts (inline, pick contact foto)
> * Contacts->SMS (Send sms)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I don't think this case has been verified carefully. T__T

> * SMS->Contacts (inline, pick contact)
> * Widget->Cost Control
> 
> and it works correctly on all of them.