1022755 - Possible race in the SMS navigation code

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

STR:

* load a reference-workload-medium on the device (tested on a flame)
* open the sms app
* open the first thread at the top of the list (MMS)
* press the back button

Expected:
* we go back to the thread list

Actual:
* the thread list is displayed for a brief instant and then we're stuck on an empty conversation panel with the header from the last conversation
* Pressing the back button doesn't do anything

Comment 1

[Julien Wajsberg [:julienw]]

I saw this intermittently without finding a good STR, so thanks for this.

I expect this is a regression of bug 881469, hence the nomination.

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Juliene I guess his one will be fall from this sprint isnt?

Comment 3

[Julien Wajsberg [:julienw]]

It's not committed for this sprint but I planned to have a look if I have time before the end of this week if I find time.

Otherwise it will wait for next sprint, yep.

Comment 4

[Julien Wajsberg [:julienw]]

I looked at it with Etienne, and it's not 100% reproducible for him either, so that's "good".

Looks like the issue is in Navigation.slide: we might have several transitionend happening when we press "back" quickly after entering a thread.

However, we're not supposed to start a new slide before the panel transition is fully finished. Therefore, I wonder if we don't get a transitionend event from "somewhere" else instead. Maybe we should check for event.target and event.propertyName in the transitionend event handling.

I think the next step would be to log stuff in [1] and then reproduce.

[1] https://github.com/mozilla-b2g/gaia/blob/82e957160ca017087bd374cd051339e55b641e68/apps/sms/js/navigation.js#L298

Comment 5

[Oleg Zasypkin [:azasypkin]]

Hey Julien,

As we discussed on IRC, I ni? you so that you can help with investigation if you have time.

Some summary of what I've already investigated:

1. This issues in fact is not regression from bug 881469, it just became more noticeable after it. I can reproduce root problem on Flame v1.4, it just looks better; 

2. Issue can be easily reproduced on Flame (master), with single thread (1 sms and 1 mms inside) by quickly tapping on Back button right after navigating to Thread panel;

3. Panels behaviour objects are initialized\deinitialized correctly, "data-position" attribute on wrapper element is correct, but "translateX(...)" is stuck while transitioning;

4. Some crazy hacky ways that may be useful to know and help to get rid of the problem (but bring another problems though): 
  4.1 make ".messages-date-group" as inline or
  4.2 use setTimeout(.., 0)\requestAnimationFrame in Navigation.slide method;

5. And just in case: once transition between panels is stuck, it can be restored via toggling "position" property for ".panel" class in AppManager. 

Thanks!

Comment 6

[Oleg Zasypkin [:azasypkin]]

One more thing that I've just noticed, quickly pressing Back button once you enter message draft sometime removes that draft from inbox (until you close and open SMS app once again).

Comment 7

[Julien Wajsberg [:julienw]]

So, the issue happens when we have MMS in the thread.

When we are in the corrupted state, this has an incorrect value:

  document.getElementById('main-wrapper').scrollLeft // = 320


This shows well that we have a scroll request somewhere.

Still investigating.

Comment 8

[Julien Wajsberg [:julienw]]

Found a fix, writing tests right now

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8449467 [details] [review]
github PR

Looks and works good! Just left few nits and question at github.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8449467 [details] [review]
github PR

Ready :)

I've added a 3rd commit with your follow-ups !

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8449467 [details] [review]
github PR

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Comment on attachment 8449467 [details] [review]
> github PR
> 
> Ready :)
> 
> I've added a 3rd commit with your follow-ups !

Great, r=me with the last tiny nit in unit test adressed!

Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Thanks !
squashed and rebased, waiting for a green-ish travis or try build

Comment 14

[Julien Wajsberg [:julienw]]

master: cb03a306869eaf2b53828512d36f11a4c1950b00

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/fe63ffdeb457fdb98fe27c138e0ba82c58712293