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

Status: RESOLVED FIXED

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

Description

[Alison Shiue]

Gaia      6b6b7d7fe829ebea85b01aa4ed44cf5ada366bbe
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/9457a67841b3
BuildID   20140703160208
Version   32.0a2

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

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

Actual result:
No shrinking UI so that the image cannot be shared

This bug similar with bug 1033964.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Ni? Francisco and michalbe for help ~

Comment 2

[Michał Budzyński (:michalbe)]

Thanks for letting us now, I'm on it now.

Comment 3

[Michał Budzyński (:michalbe)]

Attachment: Gaia part patch

I prepared a gaia part patch that should fix this bug (and sending contacts opened from the dialer as well). `Should` because when trying to share recently received contact via NFC we get a Gecko error:
'[JavaScript Error: "can't access dead object" {file: "jar:file:///system/b2g/omni.ja!/components/nsNfc.js" line: 273}]' when calling method: [nsINfcPeerCallback::peerNotification]" {file: "jar:file:///system/b2g/omni.ja!/components/NfcContentHelper.js" line: 429}]

Can someone responsible for the Gecko side of NFC take a look on this? Garner maybe?

Comment 4

[bhavana bajaj [:bajaj]]

(In reply to ashiue from comment #0)
> Gaia      6b6b7d7fe829ebea85b01aa4ed44cf5ada366bbe
> Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/9457a67841b3
> BuildID   20140703160208
> Version   32.0a2
> 
> STR:
> 1. Phone A receive contact via NFC (Phone A stay at the contact info page)
> 2. Tap 2 phones together
> 
> Expected result:
> Shrinking UI show correctly and the contact can be shared
> 
> Actual result:
> No shrinking UI so that the image cannot be shared
> 
> This bug similar with bug 1033964.

Can you confirm this is a recent regression ? i am sure we must have tested this case in the first pass of NFC?

Comment 5

[Alison Shiue]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #4)
> (In reply to ashiue from comment #0)
> > Gaia      6b6b7d7fe829ebea85b01aa4ed44cf5ada366bbe
> > Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/9457a67841b3
> > BuildID   20140703160208
> > Version   32.0a2
> > 
> > STR:
> > 1. Phone A receive contact via NFC (Phone A stay at the contact info page)
> > 2. Tap 2 phones together
> > 
> > Expected result:
> > Shrinking UI show correctly and the contact can be shared
> > 
> > Actual result:
> > No shrinking UI so that the image cannot be shared
> >  
> > This bug similar with bug 1033964.
> 
> Can you confirm this is a recent regression ? i am sure we must have tested
> this case in the first pass of NFC?

I am sorry to say that these test scenarios(this and bug 1033964) do not include in initial NFC test cases. I already added them in moztrap at the assumption that they are really bugs.

Comment 6

[Garner Lee]

Looking into it. Definately something slightly off happening. There's a techlost is being fired, but only partially removed from the callback list, and an unregister missed content side (the webapp). Patching that, the app simply doesn't have the ability to P2P share anymore until restarted (i.e. there's no callback found for CheckP2PRegistration, so the ability to share won't be offered.). P2P still works for the Browser.

I'll look at the contact app side too...

Comment 7

[Garner Lee]

Attachment: 0001-Fix-NFC-Peer-event-comparisons.patch

More of a WIP fix, for the gecko side. This ensures the callback is called, and the underlying internal NFC.ONPEERFOUND and NFC.ONPEERLOST events trigger the corresponding content callbacks. A bare debug message only callback set for onpeerfound works fine without first exiting the comms app.

However, it seems to have some trouble if the lazy loaded function is called gecko side (stale content side mozNfcPeer or related object(s)?).

Comment 8

[Ken Chang[:kenkai]]

Hi Garner, I wonder if you could file another bug for your patch. Thanks.

Comment 9

[Michał Budzyński (:michalbe)]

Garner, I'm not a Gecko dev so I don't think I'm a right person to provide feedback for this patch.

Comment 10

[Garner Lee]

Bug Created for gecko side cleanup: Bug 1037051.

Comment 11

[Ken Chang[:kenkai]]

This bug is for contacts app. We are going to tracking gecko part of NFC by using bug 1037051.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Michał Budzyński (:michalbe) from comment #3)
> Created attachment 8451032 [details] [review]
> Gaia part patch
> 
> I prepared a gaia part patch that should fix this bug (and sending contacts
> opened from the dialer as well). `Should` because when trying to share
> recently received contact via NFC we get a Gecko error:
> '[JavaScript Error: "can't access dead object" {file:
> "jar:file:///system/b2g/omni.ja!/components/nsNfc.js" line: 273}]' when
> calling method: [nsINfcPeerCallback::peerNotification]" {file:
> "jar:file:///system/b2g/omni.ja!/components/NfcContentHelper.js" line: 429}]
> 
> Can someone responsible for the Gecko side of NFC take a look on this?
> Garner maybe?

Hi Michalbe

Can you post your working Gaia branch here and give us steps to reproduce this problem ?
Does this happen all the time?

Thanks

Comment 13

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

(In reply to Yoshi Huang[:allstars.chh] from comment #12)
> (In reply to Michał Budzyński (:michalbe) from comment #3)
> > Created attachment 8451032 [details] [review]
> > Gaia part patch
> > 
> > I prepared a gaia part patch that should fix this bug (and sending contacts
> > opened from the dialer as well). `Should` because when trying to share
> > recently received contact via NFC we get a Gecko error:
> > '[JavaScript Error: "can't access dead object" {file:
> > "jar:file:///system/b2g/omni.ja!/components/nsNfc.js" line: 273}]' when
> > calling method: [nsINfcPeerCallback::peerNotification]" {file:
> > "jar:file:///system/b2g/omni.ja!/components/NfcContentHelper.js" line: 429}]
> > 
> > Can someone responsible for the Gecko side of NFC take a look on this?
> > Garner maybe?
> 
> Hi Michalbe
> 
> Can you post your working Gaia branch here and give us steps to reproduce
> this problem ?
> Does this happen all the time?
> 
> Thanks


Hi Yoshi!

Michal is on holidays, but he added the patch for gaia to enable this. Also the STR are on the first comment here:
1. Phone A receive contact via NFC (Phone A stay at the contact info page)
2. Tap 2 phones together

Thanks!

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #13)
Sorry I didn't notice he already uploaded his patch.

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Attachment: no-shrinking-after-contact-received

I've used the patch uploaded by Michał and added additional logging in Gecko. This is the logcat output from the following scenario:
1. Share contact from other device to Flame. (This is successful)
2. Not closing the current screen try to share received contact to different device. (This fails, shrinking UI is not visible)
3. Press the home button. Open contacts app, open previously received contact and try to share it with other device. (shrinking UI is visible, this is successful)

In the log we can see that patch uploaded by Michał works, callback registration after point 1. reaches Nfc.js in Gecko (line 8595):
> I/Gecko   (  282): -*- Nfc: Received 'NFC:RegisterPeerTarget' message from content process

While doing point 2. NfcManager discovers the other device and tries to get the manifest url of the current app from Shrinking UI. Log print outs starting from line 8817 show that the manifest url which NfcManager gets from Shrinking UI is null (it shouldn't be even passed to Gecko, but still is):  
>I/GeckoDump(  282): [DEBUG] SYSTEM NFC: Technology Discovered: {"type":"techDiscovered","techList":["P2P"],"records":null,"sessionToken":"{9d3ed0ee-971f-44eb-be59-4fc751741909}"}
>I/GeckoDump(  282): [DEBUG] SYSTEM NFC: KM checking for manifest 
>I/Gecko   (  282): -*- Nfc DOM: KM this is the manifest URL: null

As a result the Shrinking UI animation does not start (line 8841):
> I/GeckoDump(  282): [DEBUG] SYSTEM NFC: Error checking P2P Registration: false

This is because after the successful share in point 1. the contact details screen is an Activity not the Contact app itself, so Shrinking UI does not have the manifest set up. Alison has pointed out in comment 1 that this issue is similar to bug 1033964 and this is actually the same problem which was identified in bug 1033964 comment 7. 

Still I could not replicate the problem which Michał had in comment 3. But I think this is a different issue not related with the problem originally raised by Alison.

Comment 16

[Krzysztof Mioduszewski[:tauzen]]

Wesley, according to comment 15 and your comment here: bug 1033964 comment 17, I think this bug should not block 2.0+. What do you think?

Comment 17

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

I do agree with Krzysztof, it a rarely scenario that can be fix by just getting out from the activity, in a flow that actually doesn't make much sense.

I wouldn't block the release here, and try to properly fix it in 2.1

Cheers
F.

Comment 18

[Garner Lee]

I'm trying to repo the bug again with just this patch applied. What I see is:

1) Share from other device to Flame device. (Flame receives contact).
2) Remove, and re-establish P2P link. (Flame still on newly received contact screen).
3) From Flame, share to other device.

What happens:
On the Share/Shrink UI, it dismisses the originally shared contact, back to the contact screen, new contact is not shared.

What probably should happen:
The received contact screen should dismiss, and share the current contact.

Comment 19

[Garner Lee]

(In reply to Garner Lee from comment #18)
> I'm trying to repo the bug again with just this patch applied. What I see is:
> 
> 1) Share from other device to Flame device. (Flame receives contact).
> 2) Remove, and re-establish P2P link. (Flame still on newly received contact
> screen).
> 3) From Flame, share to other device.
> 
> What happens:
> On the Share/Shrink UI, it dismisses the originally shared contact, back to
> the contact screen, new contact is not shared.
> 
> What probably should happen:
> The received contact screen should dismiss, and share the current contact.
^^ The received contact screen should NOT dismiss, and share the current contact.

Comment 20

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

This is similar to bug 1033964, using Activity instead of the real app.
Per comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1033964#c17 , let's nominate it as 2.1? to triage later.

Comment 21

[Ken Chang[:kenkai]]

This bug is no longer in 2.1.

Comment 22

[Garner Lee]

Wait, this partial fix (to re-apply the callback to be able to share again without exiting) isn't in 2.0?

Comment 23

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

I recall that UX made decision that it should launch contact app instead of inline activity.
ni? Juwei to reconfirm this.

Comment 24

[Juwei Huang[:juwei]]

As an agreement with Wesley & Greg in comment 9 on bug 1033964, we are now looking for the possibility of solution 1: launch contact app directly from notification.

Comment 25

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

Comment on attachment 8451032 [details] [review]
Gaia part patch

Great job!

r+, relaunched the ci work in gaia-try, please land once gree.

Comment 26

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

Not a release blocker but it's a most-wanted from UX for 2.1.

Comment 27

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/96cea69d55aea6ed381c38a3c903a847f43f9645