If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

Firefox OS
FindMyDevice
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: marcia, Assigned: fabrice)

Tracking

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8454090 [details]
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
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
Assignee: nobody → 6a68
Flags: needinfo?(mozillamarcia.knous)
Target Milestone: --- → 2.0 S6 (18july)
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.
Flags: needinfo?(dougt)
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
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?
Flags: needinfo?(vkrishnamoorthy)

Comment 5

3 years ago
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.
Flags: needinfo?(vkrishnamoorthy)
(Reporter)

Comment 6

3 years ago
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.
Flags: needinfo?(mozillamarcia.knous)
(Reporter)

Comment 7

3 years ago
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.
(Reporter)

Comment 8

3 years ago
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

3 years ago
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

3 years ago
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.
(Reporter)

Updated

3 years ago
Flags: in-moztrap?(mozillamarcia.knous)
(Reporter)

Updated

3 years ago
Keywords: steps-wanted
(Reporter)

Comment 11

3 years ago
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.
(Reporter)

Updated

3 years ago
Summary: [Flame] Erase functionality fails during testing → [Flame] Erase functionality fails when a ringtone is set from the Music App
(Reporter)

Comment 12

3 years ago
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.
Summary: [Flame] Erase functionality fails when a ringtone is set from the Music App → [Flame] Phone is not completely erased when a ringtone is set from the Music App
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

3 years ago
(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

3 years ago
* 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)
(Reporter)

Comment 16

3 years ago
Created attachment 8455457 [details]
fmdlogcat.txt

New Logcat from testing done in Comment 12.
(Reporter)

Comment 17

3 years ago
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.
blocking-b2g: --- → 2.0?

Updated

3 years ago
Duplicate of this bug: 1037711

Comment 19

3 years ago
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

3 years ago
Could not reproduce Marcia's STR when instead of setting a custom Ringtone from the SD, I set a custom alert.
(Assignee)

Comment 21

3 years ago
Created attachment 8455985 [details] [diff] [review]
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);

Updated

3 years ago
blocking-b2g: 2.0? → 2.0+

Updated

3 years ago
Depends on: 1037711
(Assignee)

Comment 22

3 years ago
Created attachment 8456715 [details] [diff] [review]
patch

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

Comment 23

3 years ago
(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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(fabrice)
(Reporter)

Updated

3 years ago
Keywords: steps-wanted
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.
(Assignee)

Comment 25

3 years ago
Created attachment 8457665 [details] [diff] [review]
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.
Assignee: 6a68 → fabrice
Attachment #8455985 - Attachment is obsolete: true
Flags: needinfo?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8457665 - Flags: review?(dougt)
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.
(Assignee)

Comment 27

3 years ago
(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 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;
}
Attachment #8457665 - Flags: review?(dougt)
(Assignee)

Comment 29

3 years ago
Created attachment 8458113 [details] [diff] [review]
patch v2

I addressed all comments from Doug. Dave, can you check if you're fine with the changes?
Attachment #8456715 - Attachment is obsolete: true
Attachment #8457665 - Attachment is obsolete: true
Attachment #8458113 - Flags: review?(dhylands)
(Assignee)

Comment 30

3 years ago
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.
Attachment #8458113 - Flags: review?(khuey)
Comment on attachment 8458113 [details] [diff] [review]
patch v2

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

r=me on the webidl
Attachment #8458113 - Flags: review?(khuey) → review+
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.
Attachment #8458113 - Flags: review?(dhylands) → review+
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!!
Flags: needinfo?(ryanvm)
(Assignee)

Comment 34

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/326396f9cc4e
(Assignee)

Comment 35

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/326396f9cc4e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
Flags: needinfo?(ryanvm)
Blocks: 1040747
I can confirm this fixes the issues encountered with paused music preventing sdcard erase. Gaia FMD cleanup is happening in bug 1040747
Duplicate of this bug: 1037711

Updated

3 years ago
Flags: needinfo?(dougt)
You need to log in before you can comment on or make changes to this bug.