1060101 - [Settings][Facebook] Removing a Facebook account from Contacts will not finish if Facebook was set up during the FTU / FTE

Status: VERIFIED FIXED

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

Description

[Martin Shuman [:Marty]]

Description:
If the user signs in to a Facebook account to import contacts during the FTU, and later decides to remove that account from the settings app, the account removal process will not finish.  After removing all associated Facebook contacts, the phone will remain on a Facebook log-in page.  If the user dismisses the log-in page by clicking the 'X,' they will be stuck at an animated screen reading 'Logging out from Facebook' until they force-close the Settings app.

After restarting the Settings app, they will still be signed in to their facebook account, but will now be able to sign out properly.


Repro Steps:
1) Update a Flame to 20140828040204
2) In the FTU, sign in to a Facebook account and import contacts
3) After the FTU, navigate to the Contacts app and press the 'Gear' icon to enter settings.
4) In Contact Settings, select the Facebook account and confirm that you want to remove it.


Actual:
The user is unable to finish signing out of Facebook


Expected:
The user is able to finish signing out of Facebook

Environmental Variables:
Device: Flame Master
Build ID: 20140828040204
Gaia: 3a838afca295c9db32e1a3ec76d49fb7fe7fd2d2
Gecko: 3be45b58fc47
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0



Repro frequency: 100%
See attached: video clip (URL), logcat

Comment 1

[Martin Shuman [:Marty]]

This issue does NOT occur in Flame 2.0.
User is able to sign out of Facebook properly.

Environmental Variables:
Device: Flame 2.0
Build ID: 20140828000202
Gaia: ee5cf3aff7e66ec5e899bdffa27ef51bfa40f0f0
Gecko: 218771782328
Version: 32.0 (2.0)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Martin Shuman [:Marty]]

Attachment: logcat-Facebook.txt

Comment 3

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Functional regression, bad user experience to be unable to sign out of a facebook account for their contacts.

Requesting a window.

Comment 4

[Joshua Mitchell (Inactive)]

Mozilla Central Regression Window:

Last Working:
Device: Flame Master
Build ID: 20140822161757
Gaia: afcdd36f13e75adcdebe57d842a277fd587faf28
Gecko: 4086dddf3aec
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken:
Device: Flame Master
Build ID: 20140822163957
Gaia: e424c85eda87a40c0fa64d6a779c3fa368bf770b
Gecko: bde9525e9191
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Gaia/Gecko Swap
Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: afcdd36f13e75adcdebe57d842a277fd587faf28
Gecko: bde9525e9191
First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: e424c85eda87a40c0fa64d6a779c3fa368bf770b
Gecko: 4086dddf3aec

Gaia pushlog:https://github.com/mozilla-b2g/gaia/compare/afcdd36f13e75adcdebe57d842a277fd587faf28...e424c85eda87a40c0fa64d6a779c3fa368bf770b
--------------------------------------------------------------------------------------------------

B2G Inbound Regression Window:

Last Working:
Device: Flame Master
Build ID: 20140821231216
Gaia: 02e61f6054717699f773448907707f5246f863f6
Gecko: a6475a163389
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken:
Device: Flame Master
Build ID: 20140821232752
Gaia: e99b0c0b707470271ddc5d03b48140084f518271
Gecko: 3b687b257f7c
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Gaia/Gecko Swap
Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: 02e61f6054717699f773448907707f5246f863f6
Gecko: 3b687b257f7c
First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: e99b0c0b707470271ddc5d03b48140084f518271
Gecko: a6475a163389

Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/02e61f6054717699f773448907707f5246f863f6...e99b0c0b707470271ddc5d03b48140084f518271

--------------------------------------------------------------------------------------------------

Broken by [Contacts-FTU] When FTU import Facebook Friends the next sync cycle m…   - Adrian?

Comment 5

[Adrián de la Rosa]

I'm taking it, I will try to find what's wrong and fix it asap.

Comment 6

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: broken function which confuses user

Comment 7

[Adrián de la Rosa]

Attachment: Pull request #23758

The bug was provoked because fb_utils.js was being lazy loaded thrice, provoking some kind of interference. I didn't notice before that this file is already lazy loaded in contacts through the file fb_loader.js, so there's no need to load it elsewhere.

Comment 8

[Cristian Rodriguez (:crdlc)]

Adrian, please, take a look at LazyLoader to check if the library is being loaded several times because if you remove your files from your libraries and we change the order in the future, we could have some problem because of that

Comment 9

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8484861 [details]
Pull request #23758

Sorry Adrian, it's a r-. I prefer to not let this fix in because we don't really know why it's working, and merging this in could help hiding a bigger bug in the LazyLoader than the one we are dealing with in this issue. This needs a bit more research.

Comment 11

[Adrián de la Rosa]

Comment on attachment 8484861 [details]
Pull request #23758

PR updated. The problem was that fb_utils.js was being loaded by migrator.js before it was loaded by fb_loader.js, thus being already defined before loading parameters.js, which store the oauth flow parameters. When this happened, the url to go after logout was set to an empty string, so the event that hides the curtain and deletes the token was never triggered.

To fix it, I had to make sure that migrations were being executed after fb_loader.js was executed, so I moved them to deferred_actions.js, with the pleasant side effect of a smaller init method in contacts.js.

Comment 12

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8484861 [details]
Pull request #23758

Hi Adrian,

Thanks for your explanation. I'd prefer for Jose Manuel to review this since he's more familiar with this code.

Moving r to Jose Manuel.

Comment 13

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

(In reply to Adrián de la Rosa from comment #11)
> Comment on attachment 8484861 [details]
> Pull request #23758
> 
> PR updated. The problem was that fb_utils.js was being loaded by migrator.js
> before it was loaded by fb_loader.js, thus being already defined before
> loading parameters.js, which store the oauth flow parameters. When this
> happened, the url to go after logout was set to an empty string, so the
> event that hides the curtain and deletes the token was never triggered.
> 
> To fix it, I had to make sure that migrations were being executed after
> fb_loader.js was executed, so I moved them to deferred_actions.js, with the
> pleasant side effect of a smaller init method in contacts.js.

Wow that one was a tricky one. Good job, let's wait for Jose's review, but anyway good catch!

Comment 14

[Jose Manuel Cantera]

Comment on attachment 8484861 [details]
Pull request #23758

tests are failing on TBPL, please check

Comment 15

[Adrián de la Rosa]

Comment on attachment 8484861 [details]
Pull request #23758

Sorry about the tests. I made some minor changes to the implementation of deferred_actions.js to be able to test it properly, nothing important. I also fixed the tests and added a new test to check if we are triggering correctly the migrations now that I moved them to deferred actions. Tell me if you see something else to fix.

Thanks!

Comment 16

[Jose Manuel Cantera]

Comment on attachment 8484861 [details]
Pull request #23758

thanks Adrian. 

Landed on master

https://github.com/mozilla-b2g/gaia/commit/f33bf145bfe922320e8720b5bc1e24e6dc25754e

Comment 17

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

Comment on attachment 8484861 [details]
Pull request #23758

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Cause by splitting FTU and Contacts app
[User impact] if declined:
User won't be able to clear FB personal data from the phone
[Testing completed]:
Smoke test passed, added more unit tests
[Risk to taking this patch] (and alternatives if risky):
Low, most of the patch is related to perform some checks in an early state
[String changes made]:

Comment 18

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #17)
> Comment on attachment 8484861 [details]
> Pull request #23758
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> Cause by splitting FTU and Contacts app
> [User impact] if declined:
> User won't be able to clear FB personal data from the phone
> [Testing completed]:
> Smoke test passed, added more unit tests
> [Risk to taking this patch] (and alternatives if risky):
> Low, most of the patch is related to perform some checks in an early state
> [String changes made]:

it is already a blocker, so do we really need approval?

Comment 19

[Maria Angeles Oteo (:oteo)]

(In reply to Jose Manuel Cantera from comment #18)

> it is already a blocker, so do we really need approval?
Yes, you need it. 
https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.1

Comment 20

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

(In reply to Jose Manuel Cantera from comment #18)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #17)
> 
> it is already a blocker, so do we really need approval?

Yes, even if its a blocker

Comment 21

[Jose Manuel Cantera]

fixed in master

Comment 22

[Jason Smith [:jsmith]]

Comment on attachment 8484861 [details]
Pull request #23758

Switching to qa-approval to verify before uplift.

Josh - Can you have someone test this to see if this works or not?

Comment 23

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This no longer occurs on the latest 2.2 builds for both the v123 JB base and the v165 KK base.  There are some strange issues with the import in the FTU now.  There is no text on the grey screens for retrieving a friends list and while importing after selecting which to import.  The green title bar for friend list selection says "Service" and will highlight when pressed without effect while there is no text where "Import" would normally be.

Environmental Variables:
Device: Flame 2.2
BuildID: 20140917114258
Gaia: 72262d054ffa5d0d2b5a0033f713149281511aea
Gecko: d2c01d77b9d0
Version: 35.0a1 (2.2) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20140917114258
Gaia: 72262d054ffa5d0d2b5a0033f713149281511aea
Gecko: d2c01d77b9d0
Version: 35.0a1 (2.2) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1 uplift.

Comment 25

[Jose Manuel Cantera]

Ryan,

In order to uplift this we should first uplift bug 1056647.

Comment 26

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

(In reply to Jose Manuel Cantera from comment #25)
> Ryan,
> 
> In order to uplift this we should first uplift bug 1056647.

Then we will need to ask for approval first  :S

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/95bd5e3c011c3bffaf3f5c8e42fc81d338df625d

Comment 28

[Roland Kunkel [:RolandK]]

Verified as fixed for the latest 2.1 Flame build:

Environmental Variables:
----------------------------------------
Device: Flame 2.1
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
----------------------------------------
Device: Flame 2.2 Master
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

User can successfully sign out of Facebook after signing into Facebook during the FTE/FTU.