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

RESOLVED FIXED in 2.1 S2 (15aug)

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Alison Shiue, Assigned: michalbe)

Tracking

(Blocks: 1 bug)

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 2.0?
Ni? Francisco and michalbe for help ~
Flags: needinfo?(mbudzynski)
Flags: needinfo?(francisco)
(Assignee)

Comment 2

4 years ago
Thanks for letting us now, I'm on it now.
Flags: needinfo?(mbudzynski)
Flags: needinfo?(francisco)
(Assignee)

Updated

4 years ago
Assignee: nobody → mbudzynski
(Assignee)

Comment 3

4 years ago
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?
Attachment #8451032 - Flags: review?(francisco)
Flags: needinfo?(dgarnerlee)

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
See Also: → bug 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)
(Reporter)

Comment 5

4 years ago
(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)

Comment 6

4 years ago
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)

Comment 7

4 years ago
Created attachment 8452060 [details] [diff] [review]
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)?).
Flags: needinfo?(mbudzynski)
Target Milestone: --- → 2.0 S6 (18july)
Whiteboard: [p=1]

Comment 8

4 years ago
Hi Garner, I wonder if you could file another bug for your patch. Thanks.
Flags: needinfo?(dgarnerlee)
(Assignee)

Comment 9

4 years ago
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)

Updated

4 years ago
Depends on: 1037051

Comment 10

4 years ago
Bug Created for gecko side cleanup: Bug 1037051.

Updated

4 years ago
Attachment #8452060 - Attachment is obsolete: true
Flags: needinfo?(dgarnerlee)

Comment 11

4 years ago
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)
Created attachment 8455356 [details]
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.
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.

Updated

4 years ago
No longer depends on: 1037051

Comment 18

4 years ago
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

4 years ago
(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)

Comment 21

4 years ago
This bug is no longer in 2.1.
No longer blocks: 949293
(Reporter)

Updated

4 years ago
QA Whiteboard: [COM=NFC]

Comment 22

4 years ago
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]
status-b2g-v2.0: --- → affected
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)

Updated

4 years ago
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+] → [COM=NFC], [QAnalyst-Triage+][lead-review+]

Comment 24

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Blocks: 1059082
(Reporter)

Updated

4 years ago
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage+][lead-review+]
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.