Closed Bug 974733 Opened 6 years ago Closed 6 years ago

[Sora][Message] We should not change panels when handling the "sending" event on the window that does not send the message

Categories

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

defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 verified)

VERIFIED FIXED
1.4 S3 (14mar)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- verified
b2g-v1.3T --- fixed
b2g-v1.4 --- verified

People

(Reporter: sync-1, Assigned: julienw)

References

()

Details

(Keywords: regression, verifyme)

Attachments

(2 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
steveck
: review+
Details | Review
Firefox OS v1.3
 Mozilla build ID: 20140208004002
 
 DEFECT DESCRIPTION:
 ->The contact"A"messages display in conversation,but the sender display as contact"B".
 
  REPRODUCING PROCEDURES:
 ->There are some message in MS;
 ->Enter"Message",select a thread" A" to enter,in messages list,tap home key to idle;
 ->Enter"Call log/Contact"to select a contact "B" to send message successfully,tap home key to idle;
 ->Re-enter"Message",you can find the contact"A"messages display in conversation,but the sender display as contact"B".(KO)
  
  EXPECTED BEHAVIOUR:
 ->Should display normally
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:100%
 
  For FT PR, Please list reference mobile's behavior:Beetle lite FF is ok.
 
 lync:zhenzhen.fu.com
 7526
Does this reproduce on a 1.1 Buri device?
Flags: needinfo?(sync-1)
We recently closed as "worksforme" bug 952524 because we couldn't reproduce it, this looks related still.
(In reply to Jason Smith [:jsmith] from comment #1)
> Does this reproduce on a 1.1 Buri device?

No, I can't reproduce this bug on v1.1.
blocking-b2g: --- → 1.3?
Flags: needinfo?(sync-1)
Was unable to repro this on a sora device with BUILD 13/02/2014, can the reporter confirm this bug on the latest build ?
Flags: needinfo?(sync-1)
Dear  田旻 - 

Per discussion, please update the verification result later once you get the newest build ready on your device!

Thanks

Vance
Flags: needinfo?(tianm)
Going to pull the nom until we get confirmation this can happen on the latest build.

Also adding qawanted to see if we can reproduce this on the latest 1.3 build on Buri.
blocking-b2g: 1.3? → ---
QA Contact: selkabule
This issue reproduces on the latest Buri V1.3 Mozilla RIL

Environmental Variables:
Device: buri 1.3 MOZ RIL
BuildID: 20140225004004
Gaia: 6e883bde818d4d53aef7b5b075b4b34267918360
Gecko: e597280f9300
Version: 28.0
Firmware Version: V1.2-device.cfg

The contact"A"messages displays in the conversation,but the sender display as contact"B".
Keywords: qawanted
blocking-b2g: --- → 1.3?
Flags: needinfo?(tianm)
Flags: needinfo?(sync-1)
Regression Window:

The issue was first introduced in this build:

Environmental Variables:
Device: Buri 1.3 MOZ RIL
BuildID: 20140108004001
Gaia: 522779225e86b7a4d1bf759036678dc321a1486e
Gecko: 2287f2839256
Version: 28.0a2
Firmware Version: V1.2-device.cfg

This issue did not occur on this build:

Environmental Variables:
Device: buri 1.3 MOZ
BuildID: 20140107004001
Gaia: ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83
Gecko: 71ad7ff30010
Version: 28.0a2
Firmware Version: V1.2-device.cfg

This regression does not fall within the range of the tinderbox builds.
It can still be reproduced on version with Mozilla build ID: 20140208004002.
comment 8 is missing a last gaia/gecko vs. first gaia/gecko analysis. Please add that info.
Keywords: qawanted
This regression window 1/07-1/08 does not fall within the range of the tinderbox builds that we currently have.
Keywords: qawanted
(In reply to ktucker from comment #11)
> This regression window 1/07-1/08 does not fall within the range of the
> tinderbox builds that we currently have.

That's not what I asked for here. I asked for a last gaia working/first broken gecko & first broken gaia/last working gecko. That can be done with the above builds.
Keywords: qawanted
I'll have a look to see if there is an easy fix.
Flags: needinfo?(felash)
So, the issue is that both the inline activity window and the main window get the "sending" event from mozMobileMessage, and therefore both windows try to move to contact B.

But since we can't usually move from contact A to contact B, we don't have this path well defined in the application, that's why the move is only partially done. If the initial state is the thread list, we see the application is in contact B after the STR (which is also incorrect).

I think the regression comes from the commit where we changed from a "window" activity to an "inline" activity (bug 815093).

The regression range is indicating bug 946678 but I think this is a false positive. Before bug 946678 the STR can not be correctly done.

Now, how to fix it? I'd say we need to move the hash change in [1] to ThreadUI.onSendClick. This looks risky but is probably not that risky, because it's part of the main functions, so something that is heavily tested.

Will wait for the blocking decision before moving forward.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L60
Flags: needinfo?(felash)
We can't display a message thread for a wrong contact. blocking+
blocking-b2g: 1.3? → 1.3+
Last Working Environmental Variables:

Device: buri 1.3 MOZ RIL
BuildID: 20140107004001
Gaia: ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83
Gecko: 71ad7ff30010
Version: 28.0a2
Firmware Version: V1.2-device.cfg


First Broken Environmental Variables:

Environmental Variables:
Device: Buri 1.3 MOZ RIL 
BuildID: 20140108004001
Gaia: 522779225e86b7a4d1bf759036678dc321a1486e
Gecko: 2287f2839256
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Last Working Gaia/First Broken Gecko: Issue DOES Reproduce
Gaia: ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83
Gecko: 2287f2839256

First Broken Gaia/Last Working Gecko: Issue Does NOT Reproduce

Gaia: 522779225e86b7a4d1bf759036678dc321a1486e
Gecko: 71ad7ff30010

pushlog: https://github.com/mozilla-b2g/gaia/compare/ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83...522779225e86b7a4d1bf759036678dc321a1486e
Keywords: qawanted
Taking, I can probably do a patch tomorrow.
Assignee: nobody → felash
Salem - That isn't the right push log. The above regression window indicates this is a gecko regression, not a Gaia regression. We need the hg push log here.
Flags: needinfo?(selkabule)
Having investigated the bug it's strange that this is a gecko bug, and I see nothing interesting in the pushlog (beside the gaia bug I already talked about)
Depends on: 824717
No longer depends on: 824717
Jason, it wasn't a mistake.

Without bug 824717 I can something very very hacky and 1.3-specific but I'd rather not to.
Depends on: 824717
Drivers disagree with the approach here. I want evidence of what regressed in gecko here before we justify what actual fix we need here. bug 824717 isn't a regression, so drivers fail to see the need to include that to fix this bug.

Please bisect to find the regressing cause in gecko & justify with data on what a safe fix is needed here.
No longer depends on: 824717
Depends on: 824717
I'm quite sure Gecko didn't regress and the regression is how we handle activities in Gaia. I already said everything in comment 14 and won't repeat more.
(In reply to Julien Wajsberg [:julienw] from comment #23)
> I'm quite sure Gecko didn't regress and the regression is how we handle
> activities in Gaia. I already said everything in comment 14 and won't repeat
> more.

Sorry, but there's hard evidence above that says that's not correct. The approach being argued for is introducing more risk than needed. Drivers have already made a decision here - we want a contextual fix based on whatever regressed here. Gecko has been identified as the cause per the above testing, so we need to identify the regressing changeset.

Please identify the regressing cause to the problem & use that justify what actually should be fixed from whatever regressed. We aren't taking random extra patches here to introduce unnecessary risk.
No longer depends on: 824717
Attached file WIP Gaia PR (obsolete) —
Ok, unassigning then. I attached my WIP (doesn't work yet).

The hacky solution (in Gaia) probably involves setting a state when the user presses "send" so that we don't try to transition to the thread panel when we receive the "sending" event.

If there is a Gecko solution then fine, I'll let someone else find it because this is out of my league.
Assignee: felash → nobody
fwiw, I don't see anything in the gecko pushlog that looks related to the issue there.

Julien, can we verify if your idea about backing out bug 815093 is right?
Can we reverify the last working gaia/first broken gecko & vice versa combination again?
Keywords: qawanted
Reverting bug 815093 makes it work correctly, although the behaviour is quite different (no cross to cancel the activity, things like that). Works correctly because without Bug 815093 we use only one window, so everything stays consistent.
Taking again until Jason proves me this is a Gecko issue.
Assignee: nobody → felash
(In reply to Julien Wajsberg [:julienw] from comment #30)
> Taking again until Jason proves me this is a Gecko issue.

I'm having someone right now double check regression window.
QA Contact: selkabule → mvaughan
Attached file v1.3 Gaia PR
Here is a v1.3-specific patch. Unit tests are not ready but the code seems to work fine and fix the issue.
A little more about Bug 824717: I'm not so sure yet we could use it. The main issue is that the request's success and/or error callbacks seem to be called quite late, when the message actually left the device. It's quite late to switch the panel, and in the MMS case, I haven't checked when we actually get the success handler, it might be _really_ late.

On the other hand, adding a new state so that we know which window sent the SMS sounds wrong. I definitely want to find another solution.
(In reply to Jason Smith [:jsmith] from comment #31)
> (In reply to Julien Wajsberg [:julienw] from comment #30)
> > Taking again until Jason proves me this is a Gecko issue.
> 
> I'm having someone right now double check regression window.

I can confirm that the regression window from comment 16 is correct.

However after swapping the builds' geckos, I've found this to be a gaia issue.

Last Working Gaia/First Broken Gecko: NO REPRO
Gaia: ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83
Gecko: 2287f2839256

First Broken Gaia/Last Working Gecko: REPRO
Gaia: 522779225e86b7a4d1bf759036678dc321a1486e
Gecko: 71ad7ff30010

pushlog: https://github.com/mozilla-b2g/gaia/compare/ff5f06dd321f7d2bf4a5b311ded2c56e865d4f83...522779225e86b7a4d1bf759036678dc321a1486e
Keywords: qawanted
Target Milestone: --- → 1.4 S3 (14mar)
Attached file master Gaia PR
Attachment #8383178 - Attachment is obsolete: true
Just pushed the 2 PRs, one for master, one for v1.3, with fixed and new unit tests.

For master, using onsuccess/onerror doesn't work because they happen too late, so Bug 824717 is not useful. Still the current patch is not very good IMO and we should do something better. I have something in mind but we'll need at least Bug 881469 and other things, so it's not for now.

I still need to test this patch properly, especially MMS, and especially with multi recipients, as I'm not confident this works as expected.
Blocks: 978321
Summary: [Sora][Message]The conversation message display wrong after do some operations. → [Sora][Message] We should not change panels when answering to the "sending" event on the window that does not send the message
Summary: [Sora][Message] We should not change panels when answering to the "sending" event on the window that does not send the message → [Sora][Message] We should not change panels when handling the "sending" event on the window that does not send the message
I discussed a lot with Ayman about the case of sending a SMS to several recipients in an activity. Out of an activity, we redirect the user in the thread list (inbox) but in an activity this does not fit well as the user would be "trapped" there unless he presses home or enters another thread, because we don't have a "back" button in the thread list, and a fortiori we don't change it to a cross :)

Solutions are:
* go to the thread list and add the "cross" in the thread list: why not but it's riskier than other solutions, and too much work for a 1.3+ patch. And it's not satisfying either, because the user can enter other threads while still in the activity's window.
* stay in the "composer" window but we'd need a visual feedback that the message was correctly sent, it's also riskier, more work, and probably add l10n strings as well
* close the activity as soon as the user presses "send". Easy but from an UX point of view it may confuse the user.
* go to the thread list without the cross. This is easy too, and confusing as well, because without a cross the user is "trapped" until he enters another thread (who'd have the cross) or presses home. Therefore it's quite visible we're in a wrong state.
* go to the thread list and close the activity after a timeout (~3 sec). This is the solution Ayman and I have chosen, because this gives the user feedback and in the same time he doesn't stay trapped.

Now I'm finishing the patch and will ask a review later today.
Just want to mention that while testing I found bug 979971 related to activities.
Comment on attachment 8384635 [details] [review]
master Gaia PR

    When the user presses the "send" button, we set a boolean, so
    that when we receive the "sending" event, we know whether we're in the same
    window.

 apps/sms/js/message_manager.js                   |   7 +-
 apps/sms/js/thread_ui.js                         |  48 ++++-
 apps/sms/test/unit/message_manager_test.js       |  14 +-
 apps/sms/test/unit/mock_message_manager.js       |  15 +-
 apps/sms/test/unit/thread_ui_integration_test.js |   2 +
 apps/sms/test/unit/thread_ui_test.js             | 215 ++++++++++++++---------
 6 files changed, 191 insertions(+), 110 deletions(-)
Attachment #8384635 - Flags: review?(schung)
Thanks for the detailed description about the bug. I haven't tried it on the device but it looks good. I just left some questions on github and I'll review it tomorrow.
(In reply to Steve Chung [:steveck] from comment #40)
> Thanks for the detailed description about the bug. I haven't tried it on the
> device but it looks good. I just left some questions on github and I'll
> review it tomorrow.

I answered the questions. (quite late, sorry :( )
Comment on attachment 8384635 [details] [review]
master Gaia PR

Almost good for me, but I still have one last question(https://github.com/mozilla-b2g/gaia/pull/16798#r10420829) and a minor suggestion. Please also fix conflicts and I think we are ready for landing it.
Attachment #8384635 - Flags: review?(schung)
Answered on github, more questions from me ;)
Flags: needinfo?(schung)
My bad :(  I always fogot we have 2 instance in this case...
Flags: needinfo?(schung)
Comment on attachment 8384635 [details] [review]
master Gaia PR

Steve, is it r+ then?
Attachment #8384635 - Flags: review?(schung)
No longer blocks: 978321
Duplicate of this bug: 978321
Comment on attachment 8384635 [details] [review]
master Gaia PR

Sorry about the delay because of my misunderstanding, and thanks for the fixing!
Attachment #8384635 - Flags: review?(schung) → review+
(In reply to Julien Wajsberg [:julienw] from comment #32)
> Created attachment 8383825 [details] [review]
> v1.3 WIP Gaia PR
> 
> Here is a v1.3-specific patch. Unit tests are not ready but the code seems
> to work fine and fix the issue.

BTW you still have this WIP attachment 8383825 [details] [review] for v1.3 branch. Should we wait for this patch or we could just uplift master's patch?
Flags: in-testsuite+
(In reply to Steve Chung [:steveck] from comment #48)
> (In reply to Julien Wajsberg [:julienw] from comment #32)
> > Created attachment 8383825 [details] [review]
> > v1.3 WIP Gaia PR
> > 
> > Here is a v1.3-specific patch. Unit tests are not ready but the code seems
> > to work fine and fix the issue.
> 
> BTW you still have this WIP attachment 8383825 [details] [review] for v1.3 branch.
> Should we wait for this patch or we could just uplift master's patch?

The v1.3 PR has not been updated, I'll do this today :)

Thanks !
Comment on attachment 8383825 [details] [review]
v1.3 Gaia PR

Cherry-picked version of the latest patch, with conflicts resolved and some manual testing.

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 #): Bug 815093
[User impact] if declined: The main app instance does some actions it should not do if the user sends a SMS using an activity
[Testing completed]: yes, unit tests and manually.
[Risk to taking this patch] (and alternatives if risky): safe to medium, because we're adding some states
[String changes made]: none
Attachment #8383825 - Attachment description: v1.3 WIP Gaia PR → v1.3 Gaia PR
Attachment #8383825 - Flags: approval-gaia-v1.3?(fabrice)
Adding verifyme to check use cases of sending several messages to several people in a row, directly from the Messages app or from the Contacts app (with the Messages app beforehand killed or launched in background), using one recipient or several recipients, SMS and MMS.

Especially in v1.3 once this lands there.

Thanks!
Keywords: verifyme
master: e5041ecb1aaed54c8317ea06e0d8ecbbd0ea22c4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8383825 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Please do not uplift until we rule this out of the culprit list for Bug 982150.
Attachment #8383825 - Flags: approval-gaia-v1.3+
(In reply to Julien Wajsberg [:julienw] from comment #53)
> Please do not uplift until we rule this out of the culprit list for Bug
> 982150.

Ask for approval once you're sure you want that uplifted.
Well, I was sure...
Ok, this is not in the regression window for Bug 982150 and is good to be uplifted. Thanks.
Comment on attachment 8383825 [details] [review]
v1.3 Gaia PR

Julien is away. See comment 50 for the previous approval request.
Attachment #8383825 - Flags: approval-gaia-v1.3?(fabrice)
Attachment #8383825 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
v1.3: 1f9f248731d118b71bc3b1b9a0b5e5f6e7be93d8
Verified as fixed v1.3. This issue does NOT reproduce on the latest v1.3 Buri build:

3/26 Environmental Variables:
Device: Buri 1.3 MOZ RIL
BuildID: 20140326004002
Gaia: 812838ad0fabf51fa14435af562ddac6d26fa936
Gecko: ba97efb0da4b
Version: 28.0
Firmware Version: V1.2-device.cfg

Verified as fixed v1.4. This issue NOT reproduce on the latest v1.4 Buri build:

3/26 Environmental Variables:
Device: Buri 1.4 MOZ RIL
BuildID: 20140326000201
Gaia: 7e705dd4718d528974d99ac31866318d7e201152
Gecko: 4889124accfa
Version: 30.0a2
Firmware Version: V1.2-device.cfg

-

1) I am currently unable to verify this issue on the v1.3T Branch, leaving verifyme keyword.

2) Changing bug status to verified as this was tested against master v1.4, comment #52
Status: RESOLVED → VERIFIED
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.