1141362 - [email] ensure transitionend called async

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

Email in gaia master has a mitigation for the unexpected ontransitionend sync dispatch on a clientHeight read, as it relates to initializing the vscroll virtual scroll area for messages in the message list.

This bug is for extracting that mitigation for uplift to 2.2, since the mitigation on master was part of a larger changeset in bug 1128739 that is not going to 2.2.

The steps to reproduce are listed in bug 1135960, but I want to keep that other bug focused on the deeper question on whether the platform should sync dispatch as part of a clientHeight read, as it will likely lead to other breaks in assumptions and cause subtler bugs.

Comment 1

[James Burke [:jrburke]]

Decided to just generically fix it so that our transitionend listeners will by async notified, to avoid doubts on the platform behavior causing subtle breaks in expectations. Will put up a pull request soon that can apply to both master and 2.2.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] jrburke:bug1141362-email-async-transitionend > mozilla-b2g:master

Comment 3

[James Burke [:jrburke]]

Comment on attachment 8575546 [details] [review]
[gaia] jrburke:bug1141362-email-async-transitionend > mozilla-b2g:master

This pull request uses a new transition_end module to use a promise.then underneath to call functions that want to bind to a transitionend event.

I tested the unit and marionette tests for email locally (with a locally updated mail-fakeservers), and the tests still pass with this change.

I would like to also uplift this for 2.2, since 2.2 does not have the vscroll _initing workaround that master has. If you were concerned about risk there, that would be good to know.

After this lands on master, I would want to do a follow-on bug ticket to remove the _initing from master vscroll, since this change would address the issue, and keep the code more similar to what is on 2.2 just for our own reasoning benefits.

Comment 4

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/b84adf4acfc53915ec33cf169298c2e019398b28

Comment 5

[James Burke [:jrburke]]

Comment on attachment 8575546 [details] [review]
[gaia] jrburke:bug1141362-email-async-transitionend > mozilla-b2g:master

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1135960 tracks the core platform behavior. This is a workaround to that behavior.

[User impact] if declined:
Bug 1135960 gives steps to reproduce the error that caused the creation of this fix. Summary: going back from an activity that starts the app in compose could lead to a jumbled, unusable message list.

I expect other subtle bugs could be caused by the behavior documented in bug 1135960, this patch avoids those.

[Testing completed]:
On device and ran email marionette tests locally that test for transitions to complete.

[Risk to taking this patch] (and alternatives if risky):
Any time an async mechanism changes, best to be a bit cautious. I believe it is much safer to use this guaranteed async behavior over the behavior that exhibits in the platform now, and I do not see the bug described in bug 1135960 with this fix. I would prefer to go with this patch since we can better reason about the code in general and have more consistent expectations on how it executes.

[String changes made]:
none

Comment 6

[bhavana bajaj [:bajaj]]

James, anything QA can do to mitigate risks here pre-landing ?

Comment 7

[bhavana bajaj [:bajaj]]

Also, can you explain why this blocks 2.2 ?

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to bhavana bajaj [:bajaj] from comment #6)
> James, anything QA can do to mitigate risks here pre-landing ?

QA can perform the steps to reproduce described in bug 1135960 comment 0.

(In reply to bhavana bajaj [:bajaj] from comment #7)
> Also, can you explain why this blocks 2.2 ?

I'm not sure what James' precise rationale was here, but I think it was generally something along the lines of how the transition-end dispatch occurring synchronously as a side-effect of us trying to read clientHeight breaks an incredible number of assumptions made throughout our code-base and so is generally something we *really* want uplifted so we can support the v2.2 branch with a significantly reduced analytical overhead.

Comment 9

[John Dorlus [:Silne30]]

Thanks Andrew. I will go ahead and perform these tasks and report back.

Comment 10

[James Burke [:jrburke]]

I did the blocking-b2g thing because I thought we were in the schedule window that required it to land things on 2.2. I do think it is serious enough though that we do want an uplift to 2.2, for the reason :asuth mentioned.

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 8575546 [details] [review]
[gaia] jrburke:bug1141362-email-async-transitionend > mozilla-b2g:master

JOhn will hopefully have finished verifying this today and hence I am doing an approval in parallel in time for tomorrow's build. We'll pull back if there is any major fallouts.

Comment 12

[John Dorlus [:Silne30]]

I have been testing this for the past day and the issue does not resurface. I tested it on a number of different PVT builds and everything checks out.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/dddb86eadc227b5be984ed9f186600ebd2827200

Comment 14

[Pi Wei Cheng [:piwei] (inactive)]

The issue described at bug 1135960 comment 0 is verified as fixed on Aries, Flame 2.5, and Flame 2.2.

On Flame 2.5 319MB we have serious memory issues so sometimes it won't open email when I tap to email the Contact, sometimes it will return me to Homescreen after it has switched user to email, and sometime it succeeds transitioning me to email. In any case, it did not reproduce the issue where it would take user back to email inbox instead of Contacts.

Verified on:

Device: Aries 2.5
BuildID: 20151006110922
Gaia: 60cdaa3d3424db3432dc903e7f9c6c8fa099c06d
Gecko: 89732fcdb0baca70e8b7a25a2725117113f0db80
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Device: Flame 2.5 (319/512MB memory)
BuildID: 20151006030203
Gaia: 60cdaa3d3424db3432dc903e7f9c6c8fa099c06d
Gecko: 3edc8d4a1e198314f5d7ebd2967b85842beef602
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Device: Flame 2.2 (319MB memory)
BuildID: 20151006032504
Gaia: 5dd95cfb9f1d6501ce0e34414596ef3dd9c2f583
Gecko: fc588eb28eab
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0