1030249 - Website Commands Queue Up When FMD is Disabled, Are Executed When Re-Enabled

Status: VERIFIED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Kate Glazko]

STR:

 -Enable FMD, access website.
 -Disable FMD in Settings menu.
 -Play Sound. Nothing should happen. Enter lost mode, nothing should happen.
 -Now swipe to enable. Feel free to leave a delay between this and the previous step.
 -Your sound will play, your screen will lock.

The same behavior happens when we turn off the device/enter exit airplane mode, but fundamentally when disabling FMD, shouldn't we be preventing these commands from queuing?

Comment 1

[JR Conlin [:jrconlin,:jconlin]]

The server has no idea what state the remote device is in (Off, disabled, traveling through a tunnel, dead battery, etc.) It merely waits for the device to reconnect and then sends the commands it has pending. 

I'm happy to hear reasonable suggestions about how to handle these sorts of things, however varying policies for commands and unknown device states can become very complicated very quickly.

Comment 2

[vkrishnamoorthy]

jrconlin, ggp and I discussed this on IRC. the way forward is for the client to send FMD flag state information to the server which should prevent this behavior.

Comment 3

[JR Conlin [:jrconlin,:jconlin]]

The proposed solutions are:

FMD Toggle message
1) when the client notes that user has disabled FMD, sends a 
{enabled:false} 
message to the Server. 
2) Server notes message, changes known state of client to disabled, displays disabled controls on UI
3) When client re-enables FMD, client begins normal traffic back to Server
4) Server converts state of client to enabled, re-enables UI controls

FMD Server Pending Command collapse:
Server will collapse all pending commands to one instance of each command (e.g. one ring, one trace, one lock, etc.) The "latest" version of each command will supersede the previous version. Newer commands will be sent to the client before older ones.

Comment 4

[Erin Lancaster [:elan]]

Assigning to JR as he is working on collapse cmds on the server side.

Comment 5

[Erin Lancaster [:elan]]

Assigning to JR as he is working on collapse cmds on the server side.

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

Commands were collapsed in 
https://github.com/mozilla-services/FindMyDevice/commit/8c17fca69ea726b9f47d1073b5ece94886dc84e9

and have landed on the server. Waiting for client to provide FMD enabled status.

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: gaia pull request

This patch will notify the server when FMD is disabled, as that's necessary for the server to stop queuing commands. It also introduces some useful logging of our network activity.

Comment 8

[Guilherme Gonçalves [:ggp]]

For reference, the FMD website is https://fmd.stage.mozaws.net/

Comment 9

[:gerard-majax]

(In reply to Guilherme Gonçalves [:ggp] from comment #8)
> For reference, the FMD website is https://fmd.stage.mozaws.net/

So this is completely broken: I created an account on the stage FMD site/OAuth server (https://accounts.stage.mozaws.net/oauth/), I can login on this account from my laptop, but when I try to connect to the very same FMD from my device, it completely fails and asks me to create a new account.

Comment 10

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> (In reply to Guilherme Gonçalves [:ggp] from comment #8)
> > For reference, the FMD website is https://fmd.stage.mozaws.net/
> 
> So this is completely broken: I created an account on the stage FMD
> site/OAuth server (https://accounts.stage.mozaws.net/oauth/), I can login on
> this account from my laptop, but when I try to connect to the very same FMD
> from my device, it completely fails and asks me to create a new account.

But creating the account from the device works.

Comment 11

[Guilherme Gonçalves [:ggp]]

That sounds like bug 1029242, which seems intermittent and we thought was gone :(

Sorry about this, we'll get it working today.

Comment 12

[:gerard-majax]

Since stage servers does not seems to be working reliably since I've been assigned to review this, I don't see when we can make sure the feature works as expected.

Comment 13

[Kate Glazko]

It seems to be working well enough to test this patch today. I am running manual MozTrap tests, please see Find My Device test run for results: https://moztrap.mozilla.org/runtests/run/4586/env/27835/

Comment 14

[:gerard-majax]

(In reply to kglazko from comment #13)
> It seems to be working well enough to test this patch today. I am running
> manual MozTrap tests, please see Find My Device test run for results:
> https://moztrap.mozilla.org/runtests/run/4586/env/27835/

Yeah, but the state was really bad last week. Now it's working more reliably, I've already been able to review the other parts :)

Comment 15

[No-Jun Park [:njpark]]

Actually, just looking it from the log (and to my untrained eye), it looks like the device is receiving push notification when FMD is disabled.  When I disabled FMD, the logcat showed following when I sent Ring or Lock command. 
/GeckoDump( 1508): [findmydevice] findmydevice got push notification!
I/GeckoDump( 1508): [findmydevice] findmydevice got push notification!
I/GeckoDump( 1508): [findmydevice] got wake up request  <- I enabled here
I/GeckoDump( 1508): [findmydevice] enabled, trying to reach the server
I/GeckoDump( 1508): [findmydevice] command r, args [30]
I/GeckoDump( 1508): [findmydevice] command t, args [60]
I/GeckoDump( 1508): [findmydevice] updating location to (43.6473884, -79.3941872)

Unless there is an ack that device does not send out when a push notification is sent, and server resends them when FMD is enabled back.

Comment 16

[Guilherme Gonçalves [:ggp]]

(In reply to No-Jun Park [:njpark] from comment #15)
> Actually, just looking it from the log (and to my untrained eye), it looks
> like the device is receiving push notification when FMD is disabled.

That's expected I think. Right now (without this patch), FMD won't contact the server if it's disabled. It may still receive a push notification, but it won't do anything with it if disabled.

With the patch in this bug, it should be possible for the server to not even send the push notification, since then it will know that the device will ignore it.

Comment 17

[:gerard-majax]

And I'm again facing issues with the server side: once FMD enabled after performing a factory reset, the website does not find my device.

logcat shows a device id: d7378a7d40d73eb780b90abdf55fc4f7
when I try to reach https://find.firefox.com/#devices/d7378a7d40d73eb780b90abdf55fc4f7 it complains about unknown device.

Comment 18

[JR Conlin [:jrconlin,:jconlin]]

I believe that what's happening here is that the server is currently retaining the user's deviceid in the local session cookie. This is causing it to try and reconnect to the previously deleted device. (After a factory reset, no previous data exists on the phone, so a new deviceID is generated.) 

This problem should resolve itself once the WebUI browser session expires. We're working on a few additional potential fixes (including possibly booting the user on device erase.) These will require UX, but are not blocking FC for client work for 2.0.

Comment 19

[:gerard-majax]

It works today.

Comment 20

[:gerard-majax]

Your PR needs to be rebased, I cannot apply it.

Comment 21

[Guilherme Gonçalves [:ggp]]

PR updated.

Comment 22

[:gerard-majax]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #18)
> I believe that what's happening here is that the server is currently
> retaining the user's deviceid in the local session cookie. This is causing
> it to try and reconnect to the previously deleted device. (After a factory
> reset, no previous data exists on the phone, so a new deviceID is
> generated.) 
> 
> This problem should resolve itself once the WebUI browser session expires.
> We're working on a few additional potential fixes (including possibly
> booting the user on device erase.) These will require UX, but are not
> blocking FC for client work for 2.0.

I'm again hitting this issue ...

Comment 23

[:gerard-majax]

I cannot test the feature since I'm getting once again blocked because of this deviceid mess. Even destroying my session's cookie does not help.

Comment 24

[JR Conlin [:jrconlin,:jconlin]]

I'm a bit confused. The server has not been changed in 12 days <https://find.firefox.com/metrics/>, and you reported that things were working for you in #19. We are explicitly not updating the server code to remove any potential issues that might be introduced while we lock down the client code. 

It may be that the session cookie is not being properly deleted or that you're returning to the device URL rather than the root <https://find.firefox.com/>.

Comment 25

[:gerard-majax]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #24)
> I'm a bit confused. The server has not been changed in 12 days
> <https://find.firefox.com/metrics/>, and you reported that things were
> working for you in #19. We are explicitly not updating the server code to
> remove any potential issues that might be introduced while we lock down the
> client code. 
> 
> It may be that the session cookie is not being properly deleted or that
> you're returning to the device URL rather than the root
> <https://find.firefox.com/>.

No, I did returned to the root URL. And yes, it was working in comment 19, but in between I did a  bunch of factory reset/re-enabling the feature, for other needs.

Comment 26

[:gerard-majax]

Okay, I see the enabled flag sent to false. Looks like we are still missing some server side part, because the server still sends us requests. Re-enabling FMD after this does not trigger the requests that we received while it was off, so the feature looks to be working as expected :)

Comment 27

[:gerard-majax]

Comment on attachment 8448193 [details] [review]
gaia pull request

I commented on Github, there is mostly a couple of changes on the way asserts are written in unit test.

Once the comments are addressed, it should be okay to land!

Thanks, and sorry for the delay in being able to properly test this :(

Comment 28

[Guilherme Gonçalves [:ggp]]

PR updated, I'll be watching the tests to mark this for checkin when they're green.

Comment 29

[Guilherme Gonçalves [:ggp]]

TBPL looks good: https://tbpl.mozilla.org/?rev=df99771b0e9d1c1151a479590d0bbfa4aed71412&tree=Gaia-Try

Comment 30

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/860030fbfbb6889745711cb73790c1c2711f0bca

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/9b6d7357031f2412b18a2fb140d5c974842d4393

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
v2.0: https://github.com/mozilla-b2g/gaia/commit/b56f023ff247312a344e8f233898e803459788b9

https://tbpl.mozilla.org/php/getParsedLog.php?id=44623369&tree=Mozilla-B2g32-v2.0

Comment 34

[Mark Goodwin [:mgoodwin]]

Attachment: Gaia pull request - with unit test failure addressed

Comment 35

[:gerard-majax]

Comment on attachment 8464013 [details] [review]
Gaia pull request - with unit test failure addressed

Unit tests are okay locally on v2.0, thanks :)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/a7f5bbf42d42debfd14f65a15a7727880a32f8b2

Comment 37

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame v2.1&v2.0.
See attachment: verified_v2.0.mp4
Reproducing rate: 0/5

STR:

1.Enable FMD on deivce.
2.Access the website "find.firefox.com" on PC,sign in Fx account.
**User can play sound via tap "Ring Device",then stop ring.
3.Disable FMD in Settings.
4.Tap "Ring Device" to play sound on PC.
**No sound on deivce.
5.Enable FMD.
**No sound on deivce.
6.Tap "Ring Device" to play sound.
**Sound playing.

Flame 2.0 build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.034818
FW-Date         Thu Nov 27 03:48:29 EST 2014
Bootloader      L1TC00011880

Flame 2.1 build:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.035005
FW-Date         Thu Nov 27 03:50:16 EST 2014
Bootloader      L1TC00011880

Comment 38

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.0.MP4