Closed Bug 1034405 Opened 9 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 affected)

RESOLVED FIXED
2.1 S2 (15aug)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- affected

People

(Reporter: ashiue, Assigned: mbudzynski)

References

Details

(Whiteboard: [p=1], [2.0-flame-test-run-3] [priority])

Attachments

(2 files, 1 obsolete file)

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.
blocking-b2g: --- → 2.0?
Ni? Francisco and michalbe for help ~
Flags: needinfo?(mbudzynski)
Flags: needinfo?(francisco)
Thanks for letting us now, I'm on it now.
Flags: needinfo?(mbudzynski)
Flags: needinfo?(francisco)
Assignee: nobody → mbudzynski
Attached file 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?
Attachment #8451032 - Flags: review?(francisco)
Flags: needinfo?(dgarnerlee)
blocking-b2g: 2.0? → 2.0+
See Also: → 1033964
(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?
Flags: needinfo?(ashiue)
(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.
Flags: needinfo?(ashiue)
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...
Flags: needinfo?(dgarnerlee)
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)?).
Flags: needinfo?(mbudzynski)
Target Milestone: --- → 2.0 S6 (18july)
Whiteboard: [p=1]
Hi Garner, I wonder if you could file another bug for your patch. Thanks.
Flags: needinfo?(dgarnerlee)
Garner, I'm not a Gecko dev so I don't think I'm a right person to provide feedback for this patch.
Flags: needinfo?(mbudzynski)
Depends on: 1037051
Bug Created for gecko side cleanup: Bug 1037051.
Attachment #8452060 - Attachment is obsolete: true
Flags: needinfo?(dgarnerlee)
This bug is for contacts app. We are going to tracking gecko part of NFC by using bug 1037051.
Component: NFC → Gaia::Contacts
(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
Flags: needinfo?(mbudzynski)
(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!
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #13)
Sorry I didn't notice he already uploaded his patch.
Flags: needinfo?(mbudzynski)
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.
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?
Flags: needinfo?(whuang)
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.
No longer depends on: 1037051
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.
(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.
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.
blocking-b2g: 2.0+ → 2.1?
Flags: needinfo?(whuang)
This bug is no longer in 2.1.
No longer blocks: b2g-NFC-2.0
QA Whiteboard: [COM=NFC]
Wait, this partial fix (to re-apply the callback to be able to share again without exiting) isn't in 2.0?
QA Whiteboard: [COM=NFC] → [COM=NFC], [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [p=1] → [p=1], [2.0-flame-test-run-3]
QA Whiteboard: [COM=NFC], [QAnalyst-Triage?] → [COM=NFC], [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
I recall that UX made decision that it should launch contact app instead of inline activity.
ni? Juwei to reconfirm this.
Flags: needinfo?(jhuang)
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+] → [COM=NFC], [QAnalyst-Triage+][lead-review+]
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.
Flags: needinfo?(jhuang)
Comment on attachment 8451032 [details] [review]
Gaia part patch

Great job!

r+, relaunched the ci work in gaia-try, please land once gree.
Attachment #8451032 - Flags: review?(francisco) → review+
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Not a release blocker but it's a most-wanted from UX for 2.1.
blocking-b2g: 2.1? → backlog
Whiteboard: [p=1], [2.0-flame-test-run-3] → [p=1], [2.0-flame-test-run-3] [priority]
Landed:

https://github.com/mozilla-b2g/gaia/commit/96cea69d55aea6ed381c38a3c903a847f43f9645
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1059082
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage+][lead-review+]
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.