880115 - [SMS / MMS] Activity handling does nothing when triggering twice from Contacts app

Status: RESOLVED FIXED

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

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

I found this bug when fixing Bug 878735.

STR:
1. open message app in thread list view
2. press home
3. open contacts app and message someone
4. message app shows normally
5. back to thread list and create a new empty message
6. press home
7. open contacts app and message someone
8. contact is not filled in recipients list

And more, if we continue doing the following, it has more situation:
9. back to thread list
10. press home
11. open contacts app and message someone
12. message app still shows thread list view.


The cause of this bug:

There is a lock in activity_handler, MessageManager.activity.isLocked[2]. When message app enters thread view(new message), it is not changed back to false[1]. If there is not theadId in MessageManager.activity, it will not be unlocked[3]. All other activities will not be triggered because of locked state[2]. And more,  STR 9~12 is also caused with the same locked state. In [2], there is a github issue code, but that page is already disappeared.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L164
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L38
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L256

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

This bug is also a cause of Bug 878735. It should mark as leo?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking for now as part of the body of MMS work.

Comment 3

[Mike Pennisi [:jugglinmike]]

John: thanks for the info! This be very useful in fixing the bug.

You'll notice that some of your links now point to the wrong lines in the source code. This is because the files have changed since you wrote the original description. When copying links to specific line numbers on GitHub.com, be sure to press the "y" key on your keyboard first. This will expand the URL to point to specific revisions of the linked files.

Based on when you wrote the original description, I believe the links were relative to the file at commit bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28. I'll include the new version of the links below:

[1] https://github.com/mozilla-b2g/gaia/blob/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28/apps/sms/js/activity_handler.js#L164
[2] https://github.com/mozilla-b2g/gaia/blob/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28/apps/sms/js/activity_handler.js#L38
[3] https://github.com/mozilla-b2g/gaia/blob/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28/apps/sms/js/message_manager.js#L256

Comment 4

[Mike Pennisi [:jugglinmike]]

Attachment: Fix bug in entering application from "new" activity

I've described the most relevant details in the commit message, but here is some extra information regarding the timeline of this bug:

## September 28, 2012

- GitHub issue #5405 created, "[Contacts][SMS]Activities are launched twice" (cached version here: http://webcache.googleusercontent.com/search?q=cache:8tz67EFbmNQJ:https://github.com/mozilla-b2g/gaia/issues/5405+&cd=1&hl=en&ct=clnk&gl=us)
- Defensive lock implemented in `master` with commit 548f27a3eeebf1758379561cc4653af7e6372c62 (view on GitHub.com: https://github.com/mozilla-b2g/gaia/commit/548f27a3eeebf1758379561cc4653af7e6372c62 )

## October 1, 2012

- Last update to GitHub issue #5405
- Bug 796191 opened to track bug in Gecko

## October 2, 2012

- GitHub issue #5405 converted to BugZilla bug 796774

## October 19, 2012

- Gecko Bug 796191 resolved as "WORKSFORME"

Comment 5

[Borja Salguero [:borjasalguero]]

Taking a look!

Comment 7

[Borja Salguero [:borjasalguero]]

Comment on attachment 760596 [details] [diff] [review]
Fix bug in entering application from "new" activity

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

Basically the lock should not be removed due to the SMS App will not work as expected. Probably the bug is in other part of the code. I will try to investigate it today as well for helping you with this. Thanks!

::: apps/sms/js/activity_handler.js
@@ -34,5 @@
>    _handlers: {
>      'new': function newHandler(activity) {
>  
> -      // XXX This lock is about https://github.com/mozilla-b2g/gaia/issues/5405
> -      if (MessageManager.activity.isLocked) {

This lock can not be removed. STR with the patch:
- Go to Contact 'A' in Contacts App
- Tap quickly several times in the 'message' icon

EXPECTED:
Composer will work as expected

CURRENTLY:
Multiple activities are launched, and the composer is completely broken.

Comment 8

[Mike Pennisi [:jugglinmike]]

This sounds like a different bug than what the Lock was originally intended to address. In this case, the Gecko behavior is correct, but some applications may launch multiple Activities in response to a single intent.

Although I think using a "lock" is fragile (leading to bugs like this one), I think there is a technical reason why it cannot be used here. Consider the following usae case:

1. Open the Contact application
2. Select a contact and tap the "message" icon
3. Press the "Home" button without interacting with the Messaging application
4. Repeat steps 1 and 2

In the above case, there is no logical place to release a lock.

We should also recognize that this problem isn't limited to the "new" Activity handler. The "share" Activity (and any other future Activity the application might support) is susceptible to the same problem.

I think that we should simply throttle the activity handler. I like this approach for three reasons:

1. It solves the problem (most important, of course!)
2. It is maintainable (we don't have manage the lock across branching logic)
3. It is generalizable (it will work for *all* activity handlers)

What do you say?

Comment 9

[Borja Salguero [:borjasalguero]]

Attachment: Proposal

Comment 10

[Mike Pennisi [:jugglinmike]]

Comment on attachment 760898 [details]
Proposal

I think we have a great patch here, Borja! This should be ready for Julien's review :)

As I discussed here and on the GitHub pull request, I've created Bug 881797 to track the implementation of generic Activity handler throttling logic.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 760898 [details]
Proposal

comments on github's PR.

I think we can do better than calling onHashChange by extracting a function for this code. + unit tests please

otherwise the lock handling seems better done like this, only in this place, this feels better :)

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 760898 [details]
Proposal

to whoever comes by here, more review fixes and more reviews on github, we're getting there.

Comment 13

[Borja Salguero [:borjasalguero]]

All your suggestions added, rebased and ready to review!

Comment 15

[Borja Salguero [:borjasalguero]]

Comment on attachment 760898 [details]
Proposal

As Julien is travelling today adding Corey to this review. I've gone back to the proposal of Julien and the code is the one required by Julien in the review. Corey could you take a look? Thanks!

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 760898 [details]
Proposal

r=me with the latest comment addressed

if you want just ask me an informal last review before merging tomorrow.

I've seen another issue with the keyboard not disappearing when the "do you cant to discard the current message" alert is displayed, I'll file a bug for this.

Comment 17

[Borja Salguero [:borjasalguero]]

https://github.com/borjasalguero/gaia/commit/de6e0e4345a359a4f261eb6f3fe9fe616559d50b
https://github.com/mozilla-b2g/gaia/commit/5026dfab780b3d5205d419b9979baeebcd12bf73

Merged.

Comment 18

[Julien Wajsberg [:julienw]]

v1-train: 46ed9a47c75c6f8e9be8114ac7a819cc1efdc280

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 46ed9a47c75c6f8e9be8114ac7a819cc1efdc280