936250 - [B2G][Contacts] Canceling FB sync results in the contacts app being unresponsive.

Status: RESOLVED FIXED

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

Description

[Lucas Aliaga]

After entering in your Facebook crendentials for contacts sync, while it looks up your friends list if you tap cancel, the contacts app becomes unresponsive. Only after force closing the app through task manager/card view will the contacts app become responsive again.

Repro Steps:
1. Update Buri device to 1.3 buildID: 20131107040200.
2. Launch Contacts > Settings > Facebook Sync toggle.
3. Enter appropriate FB credentials and tap log in.
4. While it's looking up the Friends List, tap cancel.

Actual Results:
Contacts app is unresponsive.

Expected Results:
Contacts app to always be responsive after any destructive action.

Repro Rate: 5/5 100%

Environmental Variables:
Gaia   42bbe26a72e030faf07a6fc297f61a3a8ccda25b
SourceStamp 70de5e24d79b
BuildID 20131107040200
Version 28.0a1

See attached: logcat
Add'l Notes: Does not occur 1.2

Comment 1

[Matthew Vaughan]

This issue started reproducing on the 9/25 1.3 build.

- Works -
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20130924040201
Gaia: a22ba4a3a9efd99f94adf9ece8a2b391d4df295b
Gecko: 1fda74e33e06
Version: 27.0a1

- Broken -
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20130925174530
Gaia: 7e42b4d690049709c62e8783910f16ab20869f42
Gecko: fa0e6916f88c
Version: 27.0a1

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ regression

Comment 3

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 14182.html

Comment 4

[Jose Manuel Cantera]

Comment on attachment 8339985 [details]
14182.html

r- as per the comments on GH. Particularly 

E/GeckoConsole( 2802): [JavaScript Error: "SyntaxError: An invalid or illegal string was specified" {file: "app://communications.gaiamobile.org/contacts/js/importer_ui.js" line: 87}]

In addition I would feel more confortable if the patch only modifies the module/s affected by what is reported in the bug. nothing more nothing less. 

thanks

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Although you are right, in this case I think that the additional refactoring was justified :-) In fact, as you will see in the PR the problem was due to a bad brackets closing at https://github.com/mozilla-b2g/gaia/pull/14182/files#diff-092be0115b394b9124e1c344d39b8841R697 Anyhow, this update is still needed ;-)

Anyhow, as you say, I think it is safer to solve the problem the bug reports and if necessary open a new one for the additional refactoring. Consequently, I removed any changes to contacts_matching_controller.js since not affected by this bug :-)

Ready for a new review ;-)

Thanks!

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

Comments included in https://github.com/mozilla-b2g/gaia/pull/14182 New verdict needed ;-) Thanks!

Comment 7

[Jose Manuel Cantera]

Comment on attachment 8339985 [details]
14182.html

provided the indentation nits are fixed

Thanks, Germán

Comment 9

[Germán Toro del Valle (:gtorodelvalle)]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/6e59c3cd7a793303695521e387c9ce09c2ecb7aa

Comment 10

[Jason Smith [:jsmith]]

This needs to be backed out - this caused a smoketest regression in bug 949197.

Can you back this out & reland with the fix needed for bug 949197?

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Hi Jason, we are currently investigating if this bug is the real cause of the issue :-)

Comment 12

[Jose Manuel Cantera]

as we have said in the related bugs, this has nothing to do and it will not be backed out

Comment 13

[Jason Smith [:jsmith]]

(In reply to Jose M. Cantera from comment #12)
> as we have said in the related bugs, this has nothing to do and it will not
> be backed out

That's incorrect. See https://bugzilla.mozilla.org/show_bug.cgi?id=949197#c7 which explains that this is clearly the cause of this regression. The underlying problem could be a gecko bug under the hood, but this patch clearly triggered the code path to reveal the gecko bug. This needs to be backed out until the underlying gecko problem is resolved or we apply a Gaia workaround.

John - Can you back this out?

Comment 14

[Jason Smith [:jsmith]]

Discussed with Ben about some of the points above - I'm going to verify if the backout of this patch resolves the smoketest regression.

Comment 15

[Jason Smith [:jsmith]]

Looks like upon more digging that regression here happened in the JS engine per bug 697343. This is now safe to uplift.

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 6e59c3cd7a793303695521e387c9ce09c2ecb7aa to:
v1.3: abd571f4e567d4647c8b9c59f85ca293ffcd6ebf