Closed Bug 1022755 Opened 6 years ago Closed 5 years ago

Possible race in the SMS navigation code

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: etienne, Assigned: julienw)

References

Details

(Keywords: regression, Whiteboard: [p=3][not-part-of-initial-sprint])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
azasypkin
: review+
Details | Review
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
blocking-b2g: --- → 2.0?
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.
Blocks: 881469
Keywords: regression
blocking-b2g: 2.0? → 2.0+
Juliene I guess his one will be fall from this sprint isnt?
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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
Whiteboard: [p=3]
Whiteboard: [p=3] → [p=3][not-part-of-initial-sprint]
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Hardware: All → ARM
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!
Flags: needinfo?(felash)
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).
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.
Found a fix, writing tests right now
Assignee: azasypkin → felash
Flags: needinfo?(felash)
Attached file github PR
Attachment #8449467 - Flags: review?(azasypkin)
Comment on attachment 8449467 [details] [review]
github PR

Looks and works good! Just left few nits and question at github.
Comment on attachment 8449467 [details] [review]
github PR

Ready :)

I've added a 3rd commit with your follow-ups !
Flags: needinfo?(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!
Attachment #8449467 - Flags: review?(azasypkin) → review+
Flags: needinfo?(azasypkin)
Thanks !
squashed and rebased, waiting for a green-ish travis or try build
Blocks: 1033403
Blocks: sms-sprint-4
Target Milestone: --- → 2.0 S5 (4july)
master: cb03a306869eaf2b53828512d36f11a4c1950b00
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.