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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Brogan Zumwalt [Inactive]]

Attachment: 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

Comment 1

[Brogan Zumwalt [Inactive]]

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

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Broken functionality

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 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.

Comment 5

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

didn't we already handle it in https://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/Nfc.js#219?

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 7

[Krzysztof Mioduszewski[:tauzen]]

continuation from comment 6:
see here: https://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/Nfc.js#304

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 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.

Comment 9

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

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()

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

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?

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

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?

Comment 12

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

Sorry I don't think it's that complicated, add a few lines of code should make it correct and easier to understand.

Comment 13

[Krzysztof Mioduszewski[:tauzen]]

Attachment: use notifyFocusApp in notifyUserAcceptedP2P

As discussed on IRC, small refactoring in notifyFocusApp and notifyUserAcceptedP2P. (without peerTarget removal and idl changes)

Comment 14

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

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.

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (1.1) use notifyFocusApp in notifyUserAcceptedP2P

I hope this patch is simple enough. Please kindly review it.

Comment 16

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

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.

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (1.2) Introduce getFocusTabId function

Addressing comment 16.

Comment 18

[Krzysztof Mioduszewski[:tauzen]]

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b71a8925603

Comment 19

[Krzysztof Mioduszewski[:tauzen]]

Try build all green.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/38a6120cdbfb

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38a6120cdbfb

Comment 22

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (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

Comment 23

[Shally Li（leave from Mozilla）]

Per comment 21

Comment 24

[Shally Li（leave from Mozilla）]

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.

Comment 25

[Shally Li（leave from Mozilla）]

Attachment: verified_Flame_v3.0.3gp

Comment 26

[Josh Cheng [:josh]]

2.2+ as this breaks functionality.

@Norry,
Can you verify this on 2.1 to see whether this is regression?

Comment 27

[Josh Cheng [:josh]]

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

Approving as this break functionalty

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/14a1a46b54ab

Comment 29

[Shally Li（leave from Mozilla）]

(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

Comment 30

[Shally Li（leave from Mozilla）]

Attachment: video_v2.1.3gp

Comment 31

[Pi Wei Cheng [:piwei] (inactive)]

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