1037212 - [Flame] Phone is not completely erased when a ringtone is set from the Music App

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Marcia Knous [:marcia]]

Attachment: erase.txt

Flame, while running:

Gaia   1bd6e8957ccf310b2f75ba5695b058a2e284df3a
SourceStamp f0e91a6bfd1b
BuildID 20140710134018
Version 32.0a2
Base 122

Gaia   09642e74e250fbc62db860c808ef188628fca55d
SourceStamp f93c0ef45597
BuildID 20140710071928
Version 33.0a1
Base 122

STR:
1. Create a new account and login into FMD in Settings.
2. Verify the account
3. Set SD card as the default in Storage
4. Select a music file as the default ringtone.
5. Go to https://find.firefox.com/ and login
6. Test Ring - PASS
7. Test Lock - PASS
8. Test Erase - FAIL

I tested this with a master build and the latest 2.0 and both times it failed. Also tested with Open C using 2.0 build - sending the Erase Command froze the device, but when it rebooted my content was not erased.

Logcat attached

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Marcia,

I am guessing this is an invalid bug.

The log seems to indicate that the device was USB connected when you tried to erase it (usbCablePluggedIn:1). Erase does not work if the USB cable is plugged in.

Can you check that erase does/does not work with USB unplugged?

Relevant log snippet:

W/AutoMounter(  310):   PID: 1679 file: '/storage/sdcard1/Music/THE CARS ― SINCE YOU'RE GONE.MP3' app: 'Find My Device' comm: 'Find My Device' exe: '/system/b2g/plugin-container'
W/AutoMounter(  310): UpdateState: Mounted volume external has open files, not sharing or formatting
I/AutoMounter(  310): UpdateState: Volume sdcard is Mounted and inserted @ /storage/sdcard0 gen 3 locked 0 sharing en-n
I/AutoMounter(  310): UpdateState: umsAvail:1 umsEnabled:1 mode:0 usbCablePluggedIn:1 tryToShare:0
                                                                  ^^^^^^^^^^^^^^^^^^^
                                                                  looks suspicious to me

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey :dougt, I recall when I first started discussing this bug with you and dhylands, it was mentioned that we'd need to kill all open apps to ensure there were no open file handles--otherwise formatting might fail.

Is that accurate? If so, is it something we need to have for 2.0? If yes, I guess we need to figure that out right away.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Oh, :dougt, to be clear, I was reminded by these log lines in the attached logcat output:

W/AutoMounter(  310):   PID: 1679 file: '/storage/sdcard1/Music/THE CARS ― SINCE YOU'RE GONE.MP3' app: 'Find My Device' comm: 'Find My Device' exe: '/system/b2g/plugin-container'
W/AutoMounter(  310): UpdateState: Mounted volume external has open files, not sharing or formatting
I/AutoMounter(  310): UpdateState: Volume sdcard is Mounted and inserted @ /storage/sdcard0 gen 3 locked 0 sharing en-n
I/AutoMounter(  310): UpdateState: umsAvail:1 umsEnabled:1 mode:0 usbCablePluggedIn:1 tryToShare:0
I/AutoMounter(  310): UpdateState: Volume external is Mounted and inserted @ /storage/sdcard1 gen 1 locked 0 sharing dis
W/AutoMounter(  310): The following files are open under '/storage/sdcard1'
W/AutoMounter(  310):   PID: 310 file: '/storage/sdcard1/Music/THE CARS ― SINCE YOU'RE GONE.MP3' app: '' comm: 'b2g' exe: '/system/b2g/b2g'
W/AutoMounter(  310):   PID: 1679 file: '/storage/sdcard1/Music/THE CARS ― SINCE YOU'RE GONE.MP3' app: 'Find My Device' comm: 'Find My Device' exe: '/system/b2g/plugin-container'
W/AutoMounter(  310): UpdateState: Mounted volume external has open files, not sharing or formatting
I/AutoMounter(  310): UpdateState: Volume sdcard is Mounted and inserted @ /storage/sdcard0 gen 3 locked 0 sharing en-n

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Vishy, thoughts here?

If the USB cable is connected, we currently fail to erase the device.

Note that, if a device is USB connected, the attacker has access to everything in storage, the user's device has been fully compromised. I'm not sure I see blocker-level value in erasing the device after the fact.

Technically, we could set a very long timer (hours/days) to keep checking back, and erase the device at some future point, after USB is disconnected. This isn't a ton of work, but I am concerned about getting it reviewed, and I would hate to derail the whole FMD app because this IMO low-value edge case isn't handled.

What do you think?

Comment 5

[vkrishnamoorthy]

if the only condition erase fails is when the device is connected via USB, it is not a blocker for the reasons Jared mentions in comment 4. Additionally, for the device to be recognized via USB, the lockscreen has to be bypassed which would mean that the passcode is compromised too.

Comment 6

[Marcia Knous [:marcia]]

While I was testing this, I was using three different devices. This is the logcat from one of the devices. In all three cases Erase failed, I am almost positive that all three devices weren't connected to USB. I am going to retest to make sure

At the same time I was testing, Dylan also said he had some intermittent failures on the Engineering Build, and I thought Peter did as well. I asked them to provide feedback in the bug.

Comment 7

[Marcia Knous [:marcia]]

Just repeated my STR again with a fresh build (today's 2.0) and a new FX Account. I get the same result - the Erase command never reaches the device.

Comment 8

[Marcia Knous [:marcia]]

I just reexamined the phone I tested with in Comment 7. Now I see it:

* Did erase the SD card, but nothing in internal memory. The device didn't reboot when the erase command was sent from the website, but I did see the geolocation icon.
* The Song I was using as the default ringtone was on the SD card, not in internal memory
* Everything that was in internal memory is still on the device.

Comment 9

[Kate Glazko]

STR:
- Flashed new phone
- Enabled SD Card Storage
- Restarted phone
- Set an SD song as my ring tone
- Went onto FMD, played sound, worked. Locked screen, worked. Erase, waited a minute, nothing happened.
- Restarted phone, went onto FMD. Played sound, worked. Locked screen, worked. Erase, waited for more than a minute, gave up, and then my phone booted into FTU.
- Verified, SD card erased, apps erased, contacts erased.

Comment 10

[Kate Glazko]

STR (first-time success)
- Flash new phone
- Do not enable SD card storage
- Use default ring tone
- Go to FMD, play sound, worked, locked screen, worked, Erase, a bit of latency then booted into FTU.
- Verified, SD card erased, apps erased, contacts erased.

Comment 11

[Marcia Knous [:marcia]]

During the QA meeting today we discussed distilling this down to a very simple matrix. I performed a set of tests using the following criteria:

*Same Flame Device used for each test pass, reset between passes
*New mailinator email used each time
*Device is using V122 Base Image with updated fonts
*Device had SIM Card
*No Open Apps when Erase performed
*Both Internal memory and SD card had content on them
*Only the Erase Command Sent (Ring and Lost Mode were not invoked during this testing)
*Not connected to USB at any time during erase

https://docs.google.com/spreadsheets/d/1t-Mw7UgVh0zP-3RHfSiZO__96UaYYJceS95LmoAZI3w/edit#gid=0 has the results of my 4 tests done on Friday evening. As you can see the Erase passed until I introduced the ringtone change. The ringtone I selected was done so by selecting the song in the Music app and then setting it as the default.

Comment 12

[Marcia Knous [:marcia]]

Just in case it isn't clear, this is the min set of steps to reproduce (Device parameters are outlined in Comment 11)

STR:
1. Create a new FX account and login into FMD in Settings.
2. Verify the account
3. Set SD card as the default in Media Storage
4. Select a music file as the default ringtone.
5. Go to https://find.firefox.com/ and login
6. Issue only the Erase command from the website.

Expected: The entire device would be erased, and the phone would reset.
Actual: SD card erased but internal memory is not. Phone does not reset.

Comment 13

[Edwin Wong [:edwong]]

my guess would be that by setting a music file on the SD card as a dependency, an integrity check or permissions issue, throws an exception and exits the delete SD card script.

Comment 14

[Jim Porter (:squib)]

(In reply to Edwin Wong [:edwong] from comment #13)
> my guess would be that by setting a music file on the SD card as a
> dependency, an integrity check or permissions issue, throws an exception and
> exits the delete SD card script.

I believe we're actually making a *copy* of the song to put into an indexedDB in the ringtones app. That copy also gets stored as a Blob in mozSettings (assuming the song is the current ringtone). It's possible that the Blob is holding a reference to the opened file from indexedDB when it's in mozSettings, and that's causing everything to grind to a halt.

Some possible things to test to narrow this down:
* Select a built-in ringtone (other than the factory-default) and try wiping
* Set a custom ringtone from the music app, then select a built-in ringtone and try wiping
* Set a custom ringtone from the music app, then reboot and try wiping

That would help us figure out what's causing the failure.

Comment 15

[Kate Glazko]

* Set a background from an image on the SD card and try remotely wiping (to see whether this is only related to audio channels or SD card data in general)

Comment 16

[Marcia Knous [:marcia]]

Attachment: fmdlogcat.txt

New Logcat from testing done in Comment 12.

Comment 17

[Marcia Knous [:marcia]]

Since we can now reproduce this scenario reliably, nominating for 2.0.

The reason is this is a common scenario that users will do - they will change the ringtone and then the erase command will fail when they send it.

Comment 19

[Kate Glazko]

Jared's description was accurate. You don't have to even set a ring tone from the SD Card, you can just leave the music app running with the song paused and try to erase the device and it won't erase until you kill the app.

Perhaps, in Marcia's case, if an SD card song is set as a system ringtone, it's being used by the system behind-the-scenes?

Comment 20

[Kate Glazko]

Could not reproduce Marcia's STR when instead of setting a custom Ringtone from the SD, I set a custom alert.

Comment 21

[[:fabrice] Fabrice Desré]

Attachment: wip

This wip patch implements the following strategy:
- navigator.mozPower.factoryReset() now has an optional boolean parameter : when it's true we create a file under /persist before doing the factory reset.
- early on during startup, we check if this file is present, and if so we erase all the files.

I tested by changing the factory reset from the settings app, but the changes in the gaia findmydevice app look simple: the function erase() in command.js should just call navigator.mozPower.factoryReset(true);

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: patch

Works fine for me, but I would appreciate testing before asking for reviews.

Comment 23

[Marcia Knous [:marcia]]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> Created attachment 8456715 [details] [diff] [review]
> patch
> 
> Works fine for me, but I would appreciate testing before asking for reviews.

Fabrice: Is it possible to get a tryserver build with your patch?

Comment 24

[Guilherme Gonçalves [:ggp]]

I attempted to start the build on Try and got a linker error, apparently: https://tbpl.mozilla.org/?tree=Try&rev=f71f6d173851

I'll leave the b2g stuff downloading overnight to see if I can test this locally tomorrow.

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: fmd-wipe.patch

Better patch, pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=62da9fd09ad4

I change mozPower.factoryReset() to take an optional string parameter. It can be:
- "normal" for a basic factoryReset. This is the default value.
- "wipe" to trigger the post-reboot sdcard wiping. This is the one you want to use in fmd.

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8457665 [details] [diff] [review]
fmd-wipe.patch

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

::: b2g/components/RecoveryService.js
@@ +87,5 @@
> +      for (let i = 0; i < volNames.length; i++) {
> +        let name = volNames.queryElementAt(i, Ci.nsISupportsString);
> +        let volume = volumeService.getVolumeByName(name.data);
> +        log("Got volume: " + name.data + " at " + volume.mountPoint);
> +        text += "wipe " + volume.mountPoint + "\n";

Normally when we do FOTA, librecovery maps the mount points from what is seem when fxos is running to what the recovery kernel sees.

So I have a feeling that we may need something similar here.

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Dave Hylands [:dhylands] from comment #26)

> Normally when we do FOTA, librecovery maps the mount points from what is
> seem when fxos is running to what the recovery kernel sees.
> 
> So I have a feeling that we may need something similar here.

I'm not sure to understand. We are wiping from within gecko at the next reboot, so the mount points don't change.

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 8457665 [details] [diff] [review]
fmd-wipe.patch

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

nice work, mostly nits.

::: b2g/components/ProcessGlobal.js
@@ +71,5 @@
> +  cleanupAfterFactoryReset: function() {
> +    log("cleanupAfterWipe start");
> +
> +    let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +    file.initWithPath("/persist/__b2g__");

Lets use a constant for this at the top of this file -- and please call it something else, like __force_wipe__, or __toilet_paper__.  Thinking that this should be a less general file name.

@@ +83,5 @@
> +    let promise = OS.File.read("/persist/__b2g__");
> +    promise.then(
> +      (array) => {
> +        file.remove(false);
> +        this.processWipeFile(decoder.decode(array));

OOC, why wouldn't you move the decoder construction right above the processWipeFile line?

::: b2g/components/RecoveryService.js
@@ +94,5 @@
> +      Cu.import("resource://gre/modules/osfile.jsm");
> +      let encoder = new TextEncoder();
> +      let array = encoder.encode(text);
> +      let promise = OS.File.writeAtomic("/persist/__b2g__", array,
> +                                        { tmpPath: "/persist/__b2g__.tmp" });

Yeha, not sure I am in love with this name.  maybe dhylands has a better suggestion?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3348,5 @@
>    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);
>    if (vs) {
> +    nsCOMPtr<nsIArray> volNames;
> +    vs->GetVolumeNames(getter_AddRefs(volNames));
> +    uint32_t length;

assign to -1, or test for GetLength result?

::: dom/webidl/MozPowerManager.webidl
@@ +7,5 @@
>  
>  /**
> +  * The reason for the factory reset.
> +  * "normal" : simple factory reset.
> +  * "wipe"   : will also wipe external storage.

"will also attempt to wipe all user storage areas"

::: hal/Hal.h
@@ +575,5 @@
>  
>  /**
>   * Perform Factory Reset to wipe out all user data.
>   */
> +void FactoryReset(mozilla::dom::FactoryResetReason&);

I think the style here is includes the variable name:

void FactoryReset(mozilla::dom::FactoryResetReason& aReason).

::: hal/gonk/GonkHal.cpp
@@ +1707,5 @@
> +  } else if (aReason == FactoryResetReason::Wipe) {
> +    reason = "wipe";
> +  }
> +
> +  recoveryService->FactoryReset(reason);

Are you concerned about something other than the strings "normal" or "wipe" being passed?  How about:

recoveryService->FactoryReset( (aReason == FactoryResetReason::Normal) ? "normal" : "wipe);

or 

if (aReason == FactoryResetReason::Wipe) {
  recoveryService->FactoryReset("wipe");
} else {
  recoveryService->FactoryReset("normal");
}

In either case, no need for the temp var.

::: hal/sandbox/SandboxHal.cpp
@@ +433,5 @@
> +    reason.AssignLiteral("normal");
> +  } else if (aReason == FactoryResetReason::Wipe) {
> +    reason.AssignLiteral("wipe");
> +  }
> +  Hal()->SendFactoryReset(reason);

Same comment as the GonkHal.cpp.

@@ +857,5 @@
> +    if (aReason.EqualsLiteral("normal")) {
> +      reason = FactoryResetReason::Normal;
> +    } else if (aReason.EqualsLiteral("wipe")) {
> +      reason = FactoryResetReason::Wipe;
> +    }

else {
  return false;
}

Comment 29

[[:fabrice] Fabrice Desré]

Attachment: patch v2

I addressed all comments from Doug. Dave, can you check if you're fine with the changes?

Comment 30

[[:fabrice] Fabrice Desré]

Comment on attachment 8458113 [details] [diff] [review]
patch v2

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

Kyle, I'm adding a parameter to factoryReset (which is certified only) to trigger different actions in some cases.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8458113 [details] [diff] [review]
patch v2

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

r=me on the webidl

Comment 32

[Dave Hylands [:dhylands]]

Comment on attachment 8458113 [details] [diff] [review]
patch v2

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

I looked this over last night before I was asked to review, and the only comment I had was the one I made in comment 26.

Otherwise, this looks good to me.

Comment 33

[bhavana bajaj [:bajaj]]

Proctive NI on this for ryan given this is urgent and needed by 1037711(FC blockers), so you can help uplift this on 2.0 once the patch is landed on mozilla-inbound/centra :) Thank you!!

Comment 34

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/326396f9cc4e

Comment 35

[[:fabrice] Fabrice Desré]

I'm landing on aurora before merging to m-c to get it in tomorrow's nightly:
https://hg.mozilla.org/releases/mozilla-aurora/rev/f70dabed667e

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/326396f9cc4e

Comment 37

[Mark Goodwin [:mgoodwin]]

I can confirm this fixes the issues encountered with paused music preventing sdcard erase. Gaia FMD cleanup is happening in bug 1040747