Brogan Zumwalt [Inactive] Reporter
	Description • 9 years ago

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

	Brogan Zumwalt [Inactive] Reporter
	Comment 1 • 9 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

	Peter Bylenga [:PBylenga]
	Comment 2 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 3 • 9 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.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 4 • 9 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 5 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 6 • 9 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.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 7 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 8 • 9 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 9 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 10 • 9 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?

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 11 • 9 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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 12 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 13 • 9 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 14 • 9 years ago

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.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 15 • 9 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 16 • 9 years ago

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.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 17 • 9 years ago

Addressing comment 16.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 18 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 19 • 9 years ago

Try build all green.

	Pulsebot
	Comment 20 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 9 years ago

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

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 22 • 9 years ago

[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

	Shally Li（leave from Mozilla）
	Comment 23 • 9 years ago

Per comment 21

	Shally Li（leave from Mozilla）
	Comment 24 • 9 years ago

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.

	Josh Cheng [:josh]
	Comment 26 • 9 years ago

2.2+ as this breaks functionality.

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

	Josh Cheng [:josh]
	Comment 27 • 9 years ago

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

Approving as this break functionalty

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

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

	Shally Li（leave from Mozilla）
	Comment 29 • 9 years ago

(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

	Pi Wei Cheng [:piwei] (inactive)
	Comment 31 • 9 years ago

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