1059082 - [NFC] Could not share the contact which initial imported via NFC

Status: VERIFIED FIXED

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

Description

[Alison Shiue]

+++ This bug was initially created as a clone of Bug #1034405 +++

Gaia      6e804a42ab90f4251c7fe8c68731dc1c6abd8006
Gecko     https://hg.mozilla.org/mozilla-central/rev/0753f7b93ab7
BuildID   20140826181551
Version   34.0a1

STR:
1. Phone A receive contact via NFC (Phone A stay at the received contact info page)
2. Tap 2 phones together 
3. Phone A swipe shrinking UI

Expected result:
Shrinking UI show correctly and the contact can be shared

Actual result:
1. If Phone A launch contact app before receive contact via NFC, screen hang. ( http://youtu.be/hHwLA9UQV2k)
2. If Phone A launch any sharable app (except contact) before receive contact via NFC, phone A would send the content of sharable app.
3. If Phone A launch any unsharable apps or stay at homescreen before receive contact via NFC, phone A has no shrinking UI

Comment 1

[Alison Shiue]

[Blocking Requested - why for this release]:

Comment 2

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

IMHO, this shouldn't block a release, it's definitely a nice to have.

Comment 3

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

triage: strong partner request

Comment 4

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

It seems fixed in bug 1034405, but came out again?

Comment 5

[Alison Shiue]

I found this issue does not fix when I tried to verify bug 1034405.

Comment 6

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

Again, IMHO, I dont think this bug should block the release.

Comment 7

[Wilfred Mathanaraj [:WDM]]

NFC is important so I would need this addressed so we have a good user experience - remains 2.1+

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

In case of the STR result in point 1 (contacts app was opened earlier) the new contact is displayed in the inline activity opened on the top of contacts app. When trying to share this I can see that the inline activity is closed and regular contacts app screen is visible. 

What is interesting, in NfcManager logs i can see that the activity failed (onerror handler was fired), but the "import" activity defined in communications has |"returnValue": true| in manifest file. The "onerror" handler for activity seems to fire once the Shrinking UI animation starts.

Do we have some other activity fired as a results of "import"? Maybe we should consider introducing a new activity handler in manifest.webapp ("nfc-ndef-discovered"?), specifically for contact import via NFC, which would just fire the contact app instead of inline activity?

Comment 9

[Kai-Chih Hu [:khu]]

I think the target milestone is outdated. Can you update it? Thanks.

Comment 10

[Garner Lee]

Did you find out who/what fires the onerror code path?

Activities should only deliver one message at a time, and prompt the "activity chooser" if the filters match more than one. Activities currently are DOMRequests too, so an app can actually return a response.

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #8)
> What is interesting, in NfcManager logs i can see that the activity failed
> (onerror handler was fired), but the "import" activity defined in
> communications has |"returnValue": true| in manifest file. The "onerror"
> handler for activity seems to fire once the Shrinking UI animation starts.

Comment 11

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

I've been trying to dive into this issue, but when I try with 2 flames, one in 2.0 nightly and the other with current master, trying to send the contact from 2.0 to 2.2, I get the following error in the 2.2 phone:

E/GeckoConsole( 1633): Content JS LOG at app://system.gaiamobile.org/js/nfc_manager.js:545 in nm_logVisibly: [NfcManager]: CheckP2PRegistration failed

Shrinking ui appears in the first phone (2.0), but got that error on the receiver phone (2.2)

Any idea of this, should I try with 2 phones on 2.2 then?

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

|CheckP2PRegistration failed| means that 2.2 flame detected the other device, but the current foreground app didn't register onpeerready handler, so does not want to share anything to other device. I might consider changing this message to something less disturbing.

Still, if you did try to share from 2.0 (by doing a swipe) the 2.2 flame should receive it normally. Do you have problems with receiving the shared contact at all on 2.2?

I'm just doing a fresh build right now, so I can try to answer the question from comment 10.

Comment 13

[Krzysztof Mioduszewski[:tauzen]]

I've followed Garners advice and found out that the activity was actually canceled by the Contacts app itself as a result of 'visibilitychange' event basically triggered by the ShrinkingUI here [1]. I've prepared a quick fix [2] for this, after applying it I cannot replicate the error anymore and sharing works as expected. 

Francisco, I'm not familiar with the Contacts app, so I'm not sure if this won't cause errors in different scenarios. If you I think this solution is ok, I can send a pull request. If this still needs some work, please feel free to use it ;)    

[1] - https://github.com/mozilla-b2g/gaia/blob/7549939141343e23792e0a69e33225be340e9fb1/apps/system/js/shrinking_ui.js#L198
[2] - https://github.com/tauzen/gaia/commit/bb15a8102bb9425710d8e6201c541010a5d9a8ec

Comment 14

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

Attachment: Pointer to PR 24059

Removing the code to abort any activity if we are being hidden.

We used this trick to avoid some problems with keyboard that won't happen any more, so bug 1028502 will be fix as well with same solution.

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

I've tested Francisco's fix in the sharing scenarios using flame 2.2 and Android. It works fine, I cannot replicate the original bug.

Comment 16

[Jose Manuel Cantera]

Comment on attachment 8489441 [details] [review]
Pointer to PR 24059

thanks Francisco

Comment 17

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

Landed in master:

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

Comment 18

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

Comment on attachment 8489441 [details] [review]
Pointer to PR 24059

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
It's not a regression but a malfunction we have since we added the NFC functionlaity
[User impact] if declined:
User's wont be able to share repeately a contact. Major NFC feature request from partners
[Testing completed]:
Smoke test done.
[Risk to taking this patch] (and alternatives if risky):
Pretty low, in the patch we are just removing code to abort activities when contacts go to the background.
[String changes made]:

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/8f762b9dcbae9fc5f64dca21bb8feeb9a32e47fe

Comment 20

[Alison Shiue]

Verified on
[master]
Gaia-Rev        3c898380b47f298cd3b7a0dacb3a6529e94322d4
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/4cdc4b9e5832
Build-ID        20140922184244
Version         35.0a1

[2.1]
Gaia-Rev        3742913e11f69e789dcb0aa0dedf2e5572da0129
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/df42b05782aa
Build-ID        20140922185144
Version         34.0a2

Comment 21

[Sue]

Attachment: video of issue verify

This issue has been successfully verified on Flame 2.1
Steps:
1. Device A Launch Contacts app.
2. Receive a contacts from device B via NFC.
3. Shara the above contacts from device A to device B via NFC.
**It can be shara successfully and tap home key can back to home.
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141119001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141119.035246
FW-Date         Wed Nov 19 03:52:56 EST 2014
Bootloader      L1TC00011880