1028943 - Erase is not wiping data in internal memory or SD card

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Marcia Knous [:marcia]]

Flame, while running:

Gaia   5b3faed8692e0b797fb4e628281440df7fa9de12
SourceStamp 37f08ddaea48
BuildID 20140618040513
Version 33.0a1

STR:

1. Follow instructions here to get phone set up to work with website - https://wiki.mozilla.org/User_Services/Try_FMD
2. Ensure that device has internal storage set to default and there are contacts, music, etc in internal storage.
3. Perform an erase command.
4. Phone reboots, but data in internal memory is still available.

Since my SD card was not set as the default, I am not sure what is expected there, but the data on the card is there too after the erase command.

According to Bug 941289, this should be a blocker because Comment 1 mentions "Data on the device as well as SD card should be wiped."

Comment 1

[Marcia Knous [:marcia]]

Also tried with a different device, same build - set SD card as default using:

Gaia   5b3faed8692e0b797fb4e628281440df7fa9de12
SourceStamp 37f08ddaea48
BuildID 20140618040513
Version 33.0a

It appears as if it might have wiped some data on the SD card, but it didn't wipe
*Music
*a .vcf file with my contacts

Comment 2

[Edwin Wong [:edwong]]

with 'internal' set as my default storage location, no /storage/sdcard0 data was deleted.

Comment 3

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

I just tested this branch with the FMD test app:

https://github.com/guilherme-pg/gaia/compare/bug1028943-erase-internal-storage

This patch does delete photos, but does not delete contacts.

Comment 4

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

Maybe we don't want to wipe contacts? But if we do, I think we need the "permissions" key in the manifest to look like

"permissions": {
    "contacts": { "access": "readwrite" }
}

Currently it looks like

"permissions": {}

Comment 5

[Dave Hylands [:dhylands]]

You'll also need device storage permissions if you're going to delete stuff through device storage.

Comment 6

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

Sorry, to be clear, I used internal memory, not sdcard.

Comment 7

[Dave Hylands [:dhylands]]

You won't be able to wipe anything from the internal sdcard without using devicestorage (at least on some phones) - it requires root access, which is only available from the main b2g process.

Comment 8

[Doug Turner (:dougt)]

I sent ggp a patch this morning which should address the problem.

Comment 9

[Guilherme Gonçalves [:ggp]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #4)
> Maybe we don't want to wipe contacts? But if we do, I think we need the
> "permissions" key in the manifest to look like
> 
> "permissions": {
>     "contacts": { "access": "readwrite" }
> }
> 
> Currently it looks like
> 
> "permissions": {}

As far as I know, we do want to delete contacts. I haven't had a chance to try it on recent b2g, but back when I first wrote this on top of 1.1 factoryReset() did cause the contacts to be erased. I just confirmed this on a Fireweb running 1.1.0.1, and the Settings app (from where I invoked factoryReset) doesn't have permission for contacts either.

Also note that, for whatever strange reason, we have a "permissions" key inside our permissions in the manifest: https://github.com/mozilla-b2g/gaia/blob/master/apps/findmydevice/manifest.webapp#L20 . I believe what Jared saw is the innermost one, which is there by mistake, but I'd expect only the outermost to have some effect, in which case we do have device storage permissions.

Comment 10

[Guilherme Gonçalves [:ggp]]

(In reply to Guilherme Gonçalves [:ggp] from comment #9)
> I just confirmed this on a Fireweb running 1.1.0.1, and the Settings app (from where I invoked
> factoryReset) doesn't have permission for contacts either.

Of course the validity of this experiment should definitely be questioned, not only
because it's an old version of b2g, but also because I'm not sure we can expect the
behavior of factoryReset to be consistent across devices :)

Comment 11

[Guilherme Gonçalves [:ggp]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #4)
> Maybe we don't want to wipe contacts? But if we do, I think we need the
> "permissions" key in the manifest to look like
> 
> "permissions": {
>     "contacts": { "access": "readwrite" }
> }
> 

These permissions apply to the Contacts API, I filed bug 1030838 so we use it instead of relying on factoryReset. So I think we should consider this bug fixed when we're able to wipe all device storages reliably (which it looks like Doug's patch provides), and then land 1030838.

Comment 12

[Guilherme Gonçalves [:ggp]]

Attachment: gaia pull request

Attaching Doug's patch. I'd like to get a little more testing for this on the device, and perhaps it's worth writing unit tests for the change he introduces.

Comment 13

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

Stealing :)

Comment 14

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

Well, this is quite interesting.

Looks like navigator.mozPower.factoryReset deletes apps and contacts on its own, but not photos or movies. Going to fix that, then deal with sdcard.

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I don't know everything that happens when we wipe the device, but fwiw, it looks in the Android source like only /data and /cache get cleared.  The sdcards, in /storage/sdcardN, do not.

The wipe_data method in recovery.cpp [1] calls device.WipeData(), and then clears /data and /cache

Device.WipeData() is a virtual method [2] that, according to the comments, is intended for device-specific wiping actions.

[1] http://androidxref.com/4.4_r1/xref/bootable/recovery/recovery.cpp#763
[2] http://androidxref.com/4.4_r1/xref/bootable/recovery/device.h#87

Comment 16

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

Thanks for updating, jed! That AOSP/mxr stuff had my head spinning.

On Friday, I tried doing a factoryReset on device (i.e. open up devtools, type "navigator.factoryReset()", see what data is left after the phone restarts. On Flame, contacts were deleted, but photos and video were still there, so it's not really useful in the end.

I'm also not sure how android version- or vendor-specific that function could be, which is another reason to just implement erase features ourselves :-\

Comment 17

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

Attachment: Github pull 21204

Opening an updated PR after discussion with dougt

Comment 18

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

Travis looks fine[1] except for the usual one random marionette error. Merging.

[1] https://travis-ci.org/6a68/gaia/jobs/28832848

Comment 19

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

Master: https://github.com/mozilla-b2g/gaia/commit/6238bfea16fa14e7282ae3ff4280814cc7d5bdf9

Thanks for the help, dougt!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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