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

Status: VERIFIED FIXED

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

Description

[sync-1]

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

Comment 1

[Jason Smith [:jsmith]]

Does this reproduce on a 1.1 Buri device?

Comment 2

[Julien Wajsberg [:julienw]]

We recently closed as "worksforme" bug 952524 because we couldn't reproduce it, this looks related still.

Comment 3

[田旻]

(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.

Comment 4

[bhavana bajaj [:bajaj]]

Was unable to repro this on a sora device with BUILD 13/02/2014, can the reporter confirm this bug on the latest build ?

Comment 5

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Dear  田旻 - 

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

Thanks

Vance

Comment 6

[Jason Smith [:jsmith]]

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.

Comment 7

[Salem Elkabule]

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".

Comment 8

[Salem Elkabule]

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.

Comment 9

[buri.blff]

It can still be reproduced on version with Mozilla build ID: 20140208004002.

Comment 10

[Jason Smith [:jsmith]]

comment 8 is missing a last gaia/gecko vs. first gaia/gecko analysis. Please add that info.

Comment 11

[KTucker [:KTucker][Inactive 3/4/2016]]

This regression window 1/07-1/08 does not fall within the range of the tinderbox builds that we currently have.

Comment 12

[Jason Smith [:jsmith]]

(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.

Comment 13

[Julien Wajsberg [:julienw]]

I'll have a look to see if there is an easy fix.

Comment 14

[Julien Wajsberg [:julienw]]

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

Comment 15

[Jason Smith [:jsmith]]

We can't display a message thread for a wrong contact. blocking+

Comment 16

[Salem Elkabule]

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

Comment 17

[Julien Wajsberg [:julienw]]

Taking, I can probably do a patch tomorrow.

Comment 18

[Jason Smith [:jsmith]]

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.

Comment 19

[Jason Smith [:jsmith]]

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=71ad7ff30010&tochange=2287f2839256

Comment 20

[Julien Wajsberg [:julienw]]

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)

Comment 21

[Julien Wajsberg [:julienw]]

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.

Comment 22

[Jason Smith [:jsmith]]

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.

Comment 23

[Julien Wajsberg [:julienw]]

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.

Comment 24

[Jason Smith [:jsmith]]

(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.

Comment 25

[Julien Wajsberg [:julienw]]

Attachment: WIP Gaia PR

Comment 26

[Julien Wajsberg [:julienw]]

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.

Comment 27

[[:fabrice] Fabrice Desré]

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?

Comment 28

[Jason Smith [:jsmith]]

Can we reverify the last working gaia/first broken gecko & vice versa combination again?

Comment 29

[Julien Wajsberg [:julienw]]

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.

Comment 30

[Julien Wajsberg [:julienw]]

Taking again until Jason proves me this is a Gecko issue.

Comment 31

[Jason Smith [:jsmith]]

(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.

Comment 32

[Julien Wajsberg [:julienw]]

Attachment: 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.

Comment 33

[Julien Wajsberg [:julienw]]

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.

Comment 34

[Matthew Vaughan]

(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

Comment 35

[Julien Wajsberg [:julienw]]

Attachment: master Gaia PR

Comment 36

[Julien Wajsberg [:julienw]]

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.

Comment 37

[Julien Wajsberg [:julienw]]

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.

Comment 38

[Julien Wajsberg [:julienw]]

Just want to mention that while testing I found bug 979971 related to activities.

Comment 39

[Julien Wajsberg [:julienw]]

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(-)

Comment 40

[Steve Chung [:steveck]]

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.

Comment 41

[Julien Wajsberg [:julienw]]

(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 42

[Steve Chung [:steveck]]

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.

Comment 43

[Julien Wajsberg [:julienw]]

Answered on github, more questions from me ;)

Comment 44

[Steve Chung [:steveck]]

My bad :(  I always fogot we have 2 instance in this case...

Comment 45

[Julien Wajsberg [:julienw]]

Comment on attachment 8384635 [details] [review]
master Gaia PR

Steve, is it r+ then?

Comment 47

[Steve Chung [:steveck]]

Comment on attachment 8384635 [details] [review]
master Gaia PR

Sorry about the delay because of my misunderstanding, and thanks for the fixing!

Comment 48

[Steve Chung [:steveck]]

(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?

Comment 49

[Julien Wajsberg [:julienw]]

(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 50

[Julien Wajsberg [:julienw]]

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

Comment 51

[Julien Wajsberg [:julienw]]

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!

Comment 52

[Julien Wajsberg [:julienw]]

master: e5041ecb1aaed54c8317ea06e0d8ecbbd0ea22c4

Comment 53

[Julien Wajsberg [:julienw]]

Please do not uplift until we rule this out of the culprit list for Bug 982150.

Comment 54

[[:fabrice] Fabrice Desré]

(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.

Comment 55

[Julien Wajsberg [:julienw]]

Well, I was sure...

Comment 56

[Julien Wajsberg [:julienw]]

Ok, this is not in the regression window for Bug 982150 and is good to be uplifted. Thanks.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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

Julien is away. See comment 50 for the previous approval request.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

v1.3: 1f9f248731d118b71bc3b1b9a0b5e5f6e7be93d8

Comment 59

[Roland Kunkel [:RolandK]]

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