Closed
Bug 1022755
Opened 10 years ago
Closed 10 years ago
Possible race in the SMS navigation code
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:2.0+, 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)
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•10 years ago
|
blocking-b2g: --- → 2.0?
Assignee | ||
Comment 1•10 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
Updated•10 years ago
|
Comment 2•10 years ago
|
||
Juliene I guess his one will be fall from this sprint isnt?
Flags: needinfo?(felash)
Assignee | ||
Comment 3•10 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•10 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
Updated•10 years ago
|
Whiteboard: [p=3]
Updated•10 years ago
|
Whiteboard: [p=3] → [p=3][not-part-of-initial-sprint]
Updated•10 years ago
|
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Updated•10 years ago
|
Hardware: All → ARM
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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•10 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•10 years ago
|
||
Found a fix, writing tests right now
Assignee: azasypkin → felash
Flags: needinfo?(felash)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8449467 -
Flags: review?(azasypkin)
Comment 10•10 years ago
|
||
Comment on attachment 8449467 [details]
github PR
Looks and works good! Just left few nits and question at github.
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8449467 [details]
github PR
Ready :)
I've added a 3rd commit with your follow-ups !
Flags: needinfo?(azasypkin)
Comment 12•10 years ago
|
||
Comment on attachment 8449467 [details]
github PR
(In reply to Julien Wajsberg [:julienw] from comment #11)
> Comment on attachment 8449467 [details]
> 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•10 years ago
|
||
Thanks !
squashed and rebased, waiting for a green-ish travis or try build
Assignee | ||
Updated•10 years ago
|
Blocks: sms-sprint-4
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 2.0 S5 (4july)
Assignee | ||
Comment 14•10 years ago
|
||
master: cb03a306869eaf2b53828512d36f11a4c1950b00
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Comment 15•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•