Closed
Bug 880115
Opened 11 years ago
Closed 11 years ago
[SMS / MMS] Activity handling does nothing when triggering twice from Contacts app
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)
RESOLVED
FIXED
blocking-b2g | leo+ |
People
(Reporter: johnhu, Assigned: jugglinmike)
References
Details
(Whiteboard: [TD-44988])
Attachments
(1 file, 1 obsolete file)
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
Reporter | ||
Comment 1•11 years ago
|
||
This bug is also a cause of Bug 878735. It should mark as leo?
blocking-b2g: --- → leo?
Assignee | ||
Comment 3•11 years ago
|
||
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
Assignee: nobody → mike
Assignee | ||
Comment 4•11 years ago
|
||
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"
Attachment #760596 -
Flags: review?(fbsc)
Comment 5•11 years ago
|
||
Taking a look!
Comment 7•11 years ago
|
||
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.
Attachment #760596 -
Flags: review?(fbsc) → review-
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
Updated•11 years ago
|
Attachment #760898 -
Flags: feedback?(mike)
Assignee | ||
Comment 10•11 years ago
|
||
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.
Attachment #760898 -
Flags: feedback?(mike) → feedback+
Updated•11 years ago
|
Attachment #760898 -
Flags: review?(felash)
Comment 11•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
All your suggestions added, rebased and ready to review!
Updated•11 years ago
|
Attachment #760596 -
Attachment is obsolete: true
Comment 15•11 years ago
|
||
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!
Attachment #760898 -
Flags: feedback?(gnarf37)
Comment 16•11 years ago
|
||
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.
Attachment #760898 -
Flags: review?(felash) → review+
Updated•11 years ago
|
Attachment #760898 -
Flags: feedback?(gnarf37)
Comment 17•11 years ago
|
||
https://github.com/borjasalguero/gaia/commit/de6e0e4345a359a4f261eb6f3fe9fe616559d50b
https://github.com/mozilla-b2g/gaia/commit/5026dfab780b3d5205d419b9979baeebcd12bf73
Merged.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
v1-train: 46ed9a47c75c6f8e9be8114ac7a819cc1efdc280
status-b2g18:
--- → fixed
Comment 19•11 years ago
|
||
v1.1.0hd: 46ed9a47c75c6f8e9be8114ac7a819cc1efdc280
status-b2g-v1.1hd:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•