[NFC] Swiping to share browser page does not share website with second device

VERIFIED FIXED in Firefox 41, Firefox OS v2.2

Status

Firefox OS
NFC
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Brogan Zumwalt [Inactive], Assigned: tauzen)

Tracking

unspecified
FxOS-S1 (26Jun)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [3.0-Daily-Testing],[spark], URL)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8616260 [details]
Logcat

Summary (title) Field:
[NFC] Swiping to share browser page does not share website with second device

Description:
When user has two devices with NFC enabled, with Device A on a website, holding devices backs together and swiping up on NFC animation on Device A does not send browser page to Device B.

Repro Steps:
1) Update a Aries to 20150604140701
2) Enable NFC on Device A and Device B
3) Navigate to website on Device A
4) Hold backs of Device A and B together
5) Swipe up on Device A after NFC tilt animation plays

Actual:
Shrinking animation plays on Device A, but site is never received on Device B.

Expected:
Device A sends website to Device B.

Environmental Variables:
Device: Aries 3.0
Build ID: 20150604140701
Gaia: dbf8e12660af79aa118ad1c32b2efc99f9a79c7b
Gecko: 5b4c240e1a36
Gonk: 3af1ede0d0956cfbf9c549df7cd9a6807a9efdf2
Version: 41.0a1 (3.0)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/15894/
See attached: Youtube video clip & logcat
Youtube Link: https://www.youtube.com/watch?v=dFGcsz2wLLs
(Reporter)

Comment 1

2 years ago
Issue DOES reproduce on Flame 2.2, and 3.0

Shrinking animation plays on Device A, but site is never received on Device B.

Device: Flame 2.2
Build ID: 20150605002503
Gaia: 8fc797527a3eca7665bc1d1828848f2fb77ca99f
Gecko: f2157a04d75b
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0
Build ID: 20150605010203
Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3
Gecko: 0496b5b3e9ef
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: --- → affected
Flags: needinfo?(pbylenga)
[Blocking Requested - why for this release]:
Broken functionality
blocking-b2g: --- → 3.0?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
(Assignee)

Comment 3

2 years ago
From the attached logcat:
>06-05 14:48:38.547   317   317 I Gecko   : -*- NfcContentHelper: Message received: {"target":{},"name":"NFC:DOMEvent","sync":false,"json":{"tabId":22,"event":1,"sessionToken":"{7c2f9589-530e-4c2b-94e6-8c9cb2b4b49a}"},"data":{"tabId":22,"event":1,"sessionToken":"{7c2f9589-530e-4c2b-94e6-8c9cb2b4b49a}"},"objects":{}}
>06-05 14:48:38.547   317   317 I Gecko   : -*- NfcContentHelper: no listener for tabId 22

Browser is part of system app. When system app is starting up it declares peerready handler in NfcHandler for URL sharing, common for each browser window. The listener in NfcContentHelper which should dispatch these events uses system app tab/appId which is 0. 
Apparently each browser has a different tabId, in this case it's 22, but since it's part of the system app it can have only one listener (with tabId 0) created at system app startup.
Blocks: 860906
(Assignee)

Comment 4

2 years ago
Created attachment 8620530 [details] [diff] [review]
0001-passing-peerready-events-to-system-browser-if-no-lis.patch

Hi Yoshi, please take a look at comment 3. This is a workaround for the problem, but I'm not really sure if this should be the target solution (not assigning the bug to myself because of this). Still maybe this is enough for now, since I assume peerready should be replaced by peerfound in the future.
Attachment #8620530 - Flags: feedback?(allstars.chh)
(Assignee)

Updated

2 years ago
Depends on: 1173444
didn't we already handle it in https://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/Nfc.js#219?
(Assignee)

Comment 6

2 years ago
Thanks for pointing this out. I can see were the problem is right now. notifyUserAcceptedP2P uses notifyDOMEvent directly, but it does not check if the listener for this.focusApp is defined.
Assignee: nobody → kmioduszewski
(Assignee)

Comment 7

2 years ago
continuation from comment 6:
see here: https://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/Nfc.js#304
(Assignee)

Comment 8

2 years ago
Created attachment 8620867 [details] [diff] [review]
Use proper tabId in notifyUserAcceptedP2P

Hi Yoshi, can you review this?
For URL sharing to work on master, bug 1173444 needs to be also solved. I'll send my pull request with fix for it shortly.
Attachment #8620530 - Attachment is obsolete: true
Attachment #8620530 - Flags: feedback?(allstars.chh)
Attachment #8620867 - Flags: review?(allstars.chh)
Comment on attachment 8620867 [details] [diff] [review]
Use proper tabId in notifyUserAcceptedP2P

Review of attachment 8620867 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/Nfc.js
@@ +302,5 @@
>        }
>  
> +      let tabId = this.eventListeners[this.focusApp] ? this.focusApp
> +                                                     : NFC.SYSTEM_APP_ID;
> +      this.notifyDOMEvent(target, {tabId: tabId,

The patch is okay,
but I'd prefer to integrate with notifyFocusApp()
Attachment #8620867 - Flags: review?(allstars.chh) → feedback+
(Assignee)

Comment 10

2 years ago
So I actually thought about this, but notifyFocusApp uses eventListeners map and notifyUserAcceptedP2P uses peerTarget map to retrieve the target. I thought it might be better to keep this separated, since peerready will be removed in the future. What do you think?
(Assignee)

Comment 11

2 years ago
If we want to integrate this with notifyFocusApp I think we should remove the peerTarget map and all related methods (also in NfcContentHelper.js, nsNfc.js and idl) and use only eventListners map. This seems to be a bit bigger change and I would propose to do it in a separate bug. This patch will need to be uplifted to 2.2 (it's affected and this bugs breaks key functionality) I would like to keep it as small as possible. What do you think Yoshi?
Flags: needinfo?(allstars.chh)
Sorry I don't think it's that complicated, add a few lines of code should make it correct and easier to understand.
Flags: needinfo?(allstars.chh)
blocking-b2g: 3.0? → 2.2?
(Assignee)

Comment 13

2 years ago
Created attachment 8622935 [details] [diff] [review]
use notifyFocusApp in notifyUserAcceptedP2P

As discussed on IRC, small refactoring in notifyFocusApp and notifyUserAcceptedP2P. (without peerTarget removal and idl changes)
Attachment #8620867 - Attachment is obsolete: true
Attachment #8622935 - Flags: review?(allstars.chh)
Comment on attachment 8622935 [details] [diff] [review]
use notifyFocusApp in notifyUserAcceptedP2P

Review of attachment 8622935 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/Nfc.js
@@ +213,5 @@
>          }
>        });
>      },
>  
> +    notifyFocusApp: function notifyFocusApp(options, target) {

This is closer.
But target shouldn't be the 2nd argument.
It should be the 1st.
Attachment #8622935 - Flags: review?(allstars.chh)
(Assignee)

Comment 15

2 years ago
Created attachment 8622957 [details] [diff] [review]
(1.1) use notifyFocusApp in notifyUserAcceptedP2P

I hope this patch is simple enough. Please kindly review it.
Attachment #8622935 - Attachment is obsolete: true
Attachment #8622957 - Flags: review?(allstars.chh)
Comment on attachment 8622957 [details] [diff] [review]
(1.1) use notifyFocusApp in notifyUserAcceptedP2P

Review of attachment 8622957 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/Nfc.js
@@ +213,5 @@
>          }
>        });
>      },
>  
> +    notifyFocusApp: function notifyFocusApp(options, appId) {

Basically this is doing the same thing with previous version.
But this made it worse as appId now could be optional,
why would we need a options object and a optional argument?

@@ +215,5 @@
>      },
>  
> +    notifyFocusApp: function notifyFocusApp(options, appId) {
> +      let tabId = this.eventListeners[this.focusApp] ? this.focusApp
> +                                                     : NFC.SYSTEM_APP_ID;

You could move this to a function,
and that function could be used by notifyFocusApp and notifyUserAcceptedP2P to get the current focus tab.

@@ -300,5 @@
>          debug("Peer already lost or " + appId + " is not a registered PeerReadytarget");
>          return;
>        }
>  
> -      this.notifyDOMEvent(target, {tabId: this.focusApp,

then tabId: this.getFocusTabId() should do the trick.
Attachment #8622957 - Flags: review?(allstars.chh)
(Assignee)

Comment 17

2 years ago
Created attachment 8622976 [details] [diff] [review]
(1.2) Introduce getFocusTabId function

Addressing comment 16.
Attachment #8622957 - Attachment is obsolete: true
Attachment #8622976 - Flags: review?(allstars.chh)
Attachment #8622976 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 18

2 years ago
try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b71a8925603
(Assignee)

Comment 19

2 years ago
Try build all green.
Keywords: checkin-needed

Comment 20

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/38a6120cdbfb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38a6120cdbfb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 22

2 years ago
Created attachment 8623768 [details] [diff] [review]
(b2g37_v2_2 version) Introduce getFocusTabId function

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 1170053
User impact if declined:
URL sharing is not working (key functionality broken)
Testing completed:
On device testing complete.
Risk to taking this patch (and alternatives if risky):
Low
String or UUID changes made by this patch:
None
Attachment #8623768 - Flags: approval-mozilla-b2g37?
Per comment 21
status-b2g-master: affected → fixed
This bug has been verified as "pass" on latest Nightly build of Flame v3.0 by the STR in Comment 0.

Actual results: Swiping to share browser page by NFC, then test device shares the website to second device successfully.
See attachment: verified_Flame_v3.0.3gp
Reproduce rate: 0/10

Device: Flame v3.0 build(Verified)
Build ID               20150617160207
Gaia Revision          b404c41c5471c31610e64defb74ec066b411e724
Gaia Date              2015-06-17 17:01:15
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a3f280b6f8d5
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150617.192003
Firmware Date          Wed Jun 17 19:20:14 EDT 2015
Bootloader             L1TC000118D0

-----------------------------------------------------------
Keeping "Fixed" of "status-b2g-master" untill someone verifies Aries device.
Created attachment 8624015 [details]
verified_Flame_v3.0.3gp
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]

Comment 26

2 years ago
2.2+ as this breaks functionality.

@Norry,
Can you verify this on 2.1 to see whether this is regression?
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(fan.luo)

Comment 27

2 years ago
Comment on attachment 8623768 [details] [diff] [review]
(b2g37_v2_2 version) Introduce getFocusTabId function

Approving as this break functionalty
Attachment #8623768 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/14a1a46b54ab
status-b2g-v2.2: affected → fixed
status-firefox39: --- → wontfix
status-firefox40: --- → wontfix
Target Milestone: --- → FxOS-S1 (26Jun)
(In reply to Josh Cheng [:josh] from comment #26)
> @Norry,
> Can you verify this on 2.1 to see whether this is regression?

Hi Josh,

    This bug can't be repro on latest Flame v2.1 by the STR in comment 0, it can share website by NFC. So it is a regression from v2.1 to v2.2.


Please see attachment: video_v2.1.3gp.
Rate: 0/10

-------------------------------------------------------------------------------------
Device: Flame v2.1 build(unaffected)
Build ID               20150618001205
Gaia Revision          f8b848c82d1ed589f7a1eb5cc099830c867ff1d4
Gaia Date              2015-06-08 09:48:23
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0ebea88c344d
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150618.034111
Firmware Date          Thu Jun 18 03:41:22 EDT 2015
Bootloader             L1TC000118D0
status-b2g-v2.1: --- → unaffected
Flags: needinfo?(fan.luo)
Created attachment 8624578 [details]
video_v2.1.3gp
This issue is verified fixed on Flame 2.2 and 3.0 master. Swiping to share web page causes the receiving device to automatically open the shared web page.

Device: Flame (319MB full flashed KK)
BuildID: 20150619002501
Gaia: 1c33072e33c279c8aa5cb5e4a3e4da6af6cd818b
Gecko: 5ad34a170633
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame (319MB full flashed KK)
BuildID: 20150619010205
Gaia: a0df9c367a68764bdcf2e2e1c4d27f0d6ee165b8
Gecko: 2694ff2ace6a
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][MGSEI-Triage+] → [QAnalyst-Triage?][MGSEI-Triage+]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][MGSEI-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.