Possible race in the SMS navigation code

RESOLVED FIXED in 2.0 S5 (4july)

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: etienne, Assigned: julienw)

Tracking

({regression})

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
regression
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
azasypkin
: review+
Details | Review | Splinter Review
(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
blocking-b2g: --- → 2.0?
(Assignee)

Comment 1

5 years ago
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+
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Juliene I guess his one will be fall from this sprint isnt?
Flags: needinfo?(felash)
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
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).
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Found a fix, writing tests right now
Assignee: azasypkin → felash
Flags: needinfo?(felash)
(Assignee)

Comment 9

5 years ago
Created attachment 8449467 [details] [review]
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.
(Assignee)

Comment 11

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Thanks !
squashed and rebased, waiting for a green-ish travis or try build
(Assignee)

Updated

5 years ago
Blocks: 1033403
(Assignee)

Updated

5 years ago
Blocks: 1028276
(Assignee)

Updated

5 years ago
Target Milestone: --- → 2.0 S5 (4july)
(Assignee)

Comment 14

5 years ago
master: cb03a306869eaf2b53828512d36f11a4c1950b00
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
status-b2g-v2.1: affected → fixed
You need to log in before you can comment on or make changes to this bug.