935924 - [Single Variant] 3rd party apps are deleted after a factory reset

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[rafael.marquez]

****Steps
1. Installing a v1.2 build on device
2. Clone the tests repository for sigle variant: https://github.com/telefonicaid/firefoxos-gaia-testsbuild.git
3. Configuring the file variant.json
4. Clone Gaia repository and select the v1.2 branch.
5. Executing the command "GAIA_DISTRIBUTION_DIR=URL/firefoxos-gaia-testsbuild  PRODUCTION=1 make reset-gaia" to install gaia with single variant in the device
6. Complete FTE  with a movistar SIM (in this case the configured in the file .json) and verify that 3º party apps(Calculator, Poppit and Wikipedia for this case) are installed.
7. Launch a "factory reset" from Settings/Device Information/More Information

***Expected Result
After the launch the factory reset, 3º party apps continue applications 

***Actual Result
3rd party apps(Calculator, Poppit and Wikipedia) are deleted after a factory reset

Variant.json file:
{
  "apps": {
    "Twitter":
      {
        "origin": "https://mobile.twitter.com",
        "manifestURL": "https://mobile.twitter.com/cache/twitter.webapp",
        "installOrigin": "https://marketplace.firefox.com"
      },
    "Facebook":
      {
        "origin": "http://m.facebook.com",
        "manifestURL": "http://m.facebook.com/openwebapp/manifest.webapp",
        "installOrigin": "https://marketplace.firefox.com"
      },
    "Wikipedia":
      {
        "origin": "https://bits.wikimedia.org",
        "manifestURL": "https://bits.wikimedia.org/WikipediaMobileFirefoxOS/manifest.webapp",
        "installOrigin": "https://marketplace.firefox.com"
      },
    "Accuweather":
      {
        "origin": "http://m.accuweather.com",
        "manifestURL": "http://m.accuweather.com/mozilla.webapp",
        "installOrigin": "https://marketplace.firefox.com"
      },
    "Calculator":
      {
        "origin": "app://calculator",
        "installOrigin": "https://marketplace.firefox.com",
        "manifestURL": "https://marketplace.firefox.com/app/9f96ce77-5b2d-42ca-a0d9-10a933dd84c4/manifest.webapp"
      },
    "Poppit":
      {
        "origin": "app://poppit",
        "installOrigin": "https://marketplace.firefox.com",
        "manifestURL": "https://marketplace.firefox.com/app/d37cb5ed-525a-408e-a7cf-ec848064e041/manifest.webapp"
      }
  },
  "operators": [
    {
      "id": "movistar",
      "mcc-mnc": [
        "214-001",
        "214-002",
        "214-005",
        "214-007"
      ],
      "apps": [
        {
          "id": "Calculator",
          "screen": 1,
          "location": 2
        },
        {
          "id": "Poppit",
          "screen": 1,
          "location": 3
        },
        {
          "id": "Wikipedia",
          "screen": 2,
          "location": 2
        }
      ],
      "support_contacts": "resources/support_contacts_movistar.json",
      "default_contacts": "resources/contacts_movistar.json",
      "ringtone": "resources/Movistar_Mid_ABR_128kbps.ogg",
      "wallpaper": "resources/customize.jpg"
    },
    {
      "id": "mexico",
      "mcc-mnc": [
        "334-004"
      ],
      "apps": [
        {
          "id": "Twitter",
          "screen": 1,
          "location": 2
        },
        {
          "id": "Facebook",
          "screen": 1,
          "location": 3
        },
        {
          "id": "Accuweather",
          "screen": 2,
          "location": 3
        },
        {
          "id": "Wikipedia",
          "screen": 2,
          "location": 2
        }
      ],
      "ringtone": "resources/ringer_dream_theme.ogg",
      "wallpaper": "resources/customize2.png"
    }
  ]
}

Comment 1

[Jason Smith [:jsmith]]

Do we know what happens with 3rd party preinstalled apps in the old customization format in 1.1 when you do a factory reset?

Comment 2

[Marcelino Veiga Tuimil [:sonmarce]]

In version 1.1 there is no local 3rd party apps, it was added in version 1.2 for single variant

Comment 3

[Jason Smith [:jsmith]]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #2)
> In version 1.1 there is no local 3rd party apps, it was added in version 1.2
> for single variant

No that's not right. Local 3rd party apps have existed since 1.01. The SIM customization variant support was introduced in 1.2.

Comment 4

[Marcelino Veiga Tuimil [:sonmarce]]

Well, maybe they have the same name, but different meaning. In version 1.2 we have local 3rd party apps per MCC/MNC, to be installed the first time a SIM card is inserted. This behavior did not exist in version 1.1.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #4)
> Well, maybe they have the same name, but different meaning. In version 1.2
> we have local 3rd party apps per MCC/MNC, to be installed the first time a
> SIM card is inserted. This behavior did not exist in version 1.1.

Well right - but what I'm trying to understand here is if this bug exists in the 1.1 implementation of preinstalled apps. That will effect whether we block on this or not.

Comment 6

[Carmen Jimenez Cabezas]

I don't think this is a preexisting problem and in fact I don't think this will affect at all apps that are not single variant. 

External (third party) global apps are preinstalled on the phone at build time. The single variant apps on the other hand are installed at runtime. 

In any case, it should work since after the reset the first time the device is booted the apps should be reinstalled. I can think of two reasons why this could be failing:

1. The reset is erasing the whole data directory. That would erase also the single variant staging directory and thus the single variant apps would be lost. 
2. The SIM has changed between the original boot and the reset.

The most probable cause is 1. I'm taking this and if that's the case will patch the reset so it doesn't erase the single variant directory.

Comment 7

[Jason Smith [:jsmith]]

Okay, thanks for the clarification then. Removing needinfo then - sounds like we know we need to get the factory reset use case fixed.

Comment 8

[Carmen Jimenez Cabezas]

I've been checking the code and we have a problem: 
The reset code is native (non open source) code. Apparently what we're doing at is just loading an external library file which is the one that actually does the reset. And it seems to be erasing the whole /data partition. Thus, the single variant configuration directory is erased. 

A easy solution (that requires two patches, one on the build system and other on the Gecko code) would be to move the sv configuration directory to /system. The problem with that is that it won't be possible to fulfill the requisite of erasing the non used apps.

There's something else we can try, which is checking what do that external library actually does, and check if it's configurable somehow. 

Adding our product people to the loop also.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Carmen Jimenez Cabezas from comment #8)
> I've been checking the code and we have a problem: 
> The reset code is native (non open source) code. Apparently what we're doing
> at is just loading an external library file which is the one that actually
> does the reset. And it seems to be erasing the whole /data partition. Thus,
> the single variant configuration directory is erased. 
> 
> A easy solution (that requires two patches, one on the build system and
> other on the Gecko code) would be to move the sv configuration directory to
> /system. The problem with that is that it won't be possible to fulfill the
> requisite of erasing the non used apps.

Why can't the non-used apps be erased?

Comment 10

[Carmen Jimenez Cabezas]

(In reply to Jason Smith [:jsmith] from comment #9)

> 
> Why can't the non-used apps be erased?

Because /system is read-only directory

Comment 11

[Jason Smith [:jsmith]]

Discussed offline - this isn't a blocker for release. The only requirement for preloading is that the apps are preloaded correctly during first run of the phone. There isn't a requirement that factory reset has to keep the preloaded apps. However, this use case does make sense to support, so this is a valid bug to consider getting fixed.

Comment 12

[Karen Ward [:kward]]

(In reply to Jason Smith [:jsmith] from comment #11)
> Discussed offline - this isn't a blocker for release. The only requirement
> for preloading is that the apps are preloaded correctly during first run of
> the phone. There isn't a requirement that factory reset has to keep the
> preloaded apps. However, this use case does make sense to support, so this
> is a valid bug to consider getting fixed.

Agree with Jason.

Comment 13

[Beatriz Rodríguez [:brg]]

This is a blocker in all Telefonica countries. We cannot face the calls to the customer care service department if we do not change this behaviour.

Comment 14

[Karen Ward [:kward]]

David - will you provide information as to whether the current behavior of the factory reset will negatively impact users who have purchased apps?  In other words, will they loose access to apps they have purchased? (or wiill the receipts persist allowing the end user to use the apps they have already paid for?)

Comment 15

[Jason Smith [:jsmith]]

(In reply to Karen Ward [:kward] from comment #14)
> David - will you provide information as to whether the current behavior of
> the factory reset will negatively impact users who have purchased apps?  In
> other words, will they loose access to apps they have purchased? (or wiill
> the receipts persist allowing the end user to use the apps they have already
> paid for?)

This bug has no relation to paid apps - none of our preinstalled apps are paid.

Comment 16

[David Bialer [:dbialer]]

For purchased apps through the marketplace, these should not be lost.  Using the same login that they used, they should see the app on their "My Apps" tab with an Install button.

Comment 17

[joseenrique.alvarezfernandez]

Agree with Beatriz. All preloaded apps must remain after a factory reset.

Comment 18

[Simon Callan, Telefonica]

Just to add one last comment - we also have contractual commitments with 3rd parties so there is a commercial implication here as well

Comment 19

[Jason Smith [:jsmith]]

Mentioned this in System FE Planning:

This needs a product call on what to do here.

Comment 20

[Peter Dolanjski [:pdol]]

I think we're going to have to block on this.  It does sound like a factory reset would indeed be expected to bring the device back to the state it was in when it left the factory, whereby completing the FTE after reset would result in the preloads being shown to the user based on the SIM.

Comment 21

[Beatriz Rodríguez [:brg]]

(In reply to Peter Dolanjski [:pdol] from comment #20)
> I think we're going to have to block on this.  It does sound like a factory
> reset would indeed be expected to bring the device back to the state it was
> in when it left the factory, whereby completing the FTE after reset would
> result in the preloads being shown to the user based on the SIM.
Thanks a lot Peter for your good understanding.

Comment 22

[Preeti Raghunath(:Preeti)]

Gregor,

Do we have an update here? Is SystemFE team looking at this?

Comment 23

[Preeti Raghunath(:Preeti)]

Carmen,

What are the next steps here? What would be the ETA for the fix?

Comment 24

[Marcelino Veiga Tuimil [:sonmarce]]

Carmen already started to work on it, she will give more feedback today

Comment 25

[Carmen Jimenez Cabezas]

Sorry for the delay, I going to upload a patch for this bug tomorrow.

Comment 26

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v1

The present bug solution is that we move the singleVariant files to a partition which it is not formatted in factory reset proccess and where we have write permisions.
With the objective of affecting as little as possible the build proccess, the SingleVariant directory will be initially instaled on the data partition.
The first time the phone boot, it will look up in the persist partition the singlevariantconf.json file. If it doesn't exist then it will search for the singlevariant apps in /data/local and if it exists, it will move the singlevariant dir to the persist partition. After that it will install the apps from the persist partition.

The persist partition will be configured in a preference.

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8336171 [details] [diff] [review]
Proposed patch v1

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

::: b2g/app/b2g.js
@@ +435,5 @@
>  #endif
>  
> +#ifdef MOZ_B2G_RIL
> +// SingleVariant
> +pref("dom.webapps.singleVariantSourceDir", "/persist/svoperapps");

Nit: we use snake_case in preferences, so single_variant_sourcedir

::: dom/apps/src/OperatorApps.jsm
@@ +140,5 @@
> +        }
> +      }
> +    } catch (e) {
> +      debug("Error copying " + aOrg.path + " to " + aDst.path + ". " + e);
> +    }

Please rewrite this using https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.DirectoryIterator_for_the_main_thread

Comment 28

[Carmen Jimenez Cabezas]

I don't make the copy proccess asyncronous intentionally because this proccess will only run in the first boot of the telephone. The move proccess won't even be executed on factory resets. And I want to ensure that the procces has finished before continuing because the sv apps installation proccess must ever start after the apps are in the correct location.

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Carmen Jimenez Cabezas from comment #28)
> I don't make the copy proccess asyncronous intentionally because this
> proccess will only run in the first boot of the telephone. The move proccess
> won't even be executed on factory resets. And I want to ensure that the
> procces has finished before continuing because the sv apps installation
> proccess must ever start after the apps are in the correct location.

You can wrap the OS.File calls with a Task to get this behavior. That would be much better than doing main thread i/o.

Comment 30

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v2

changes requested

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8339262 [details] [diff] [review]
Proposed patch v2

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

r=me with comments addressed

::: b2g/app/b2g.js
@@ +440,5 @@
>  #endif
>  
> +#ifdef MOZ_B2G_RIL
> +// SingleVariant
> +pref("dom.webapps.single_variant_sourcedir", "/persist/svoperapps");

we use dom.mozApps. as a prefix in preferences for this code.

::: dom/apps/src/OperatorApps.jsm
@@ +40,4 @@
>  const SINGLE_VARIANT_SOURCE_DIR = "svoperapps";
>  const SINGLE_VARIANT_CONF_FILE  = "singlevariantconf.json";
>  const PREF_FIRST_RUN_WITH_SIM   = "dom.webapps.firstRunWithSIM";
> +const PREF_SINGLE_VARIANT_DIR   = "dom.webapps.single_variant_sourcedir";

Update the pref name here to.

@@ +131,5 @@
> +  _copyDirectory: function(aOrg, aDst) {
> +    return Task.spawn(function() {
> +      try {
> +        let orgInfo = yield File.stat(aOrg);
> +        if (!aDst || !orgInfo.isDir) {

nit: you could return earlier if (!aDst).

@@ +171,5 @@
> +  },
> +
> +  _initializeSourceDir: function() {
> +    return Task.spawn(function() {
> +      var svFinalDirName;

s/var/let

@@ +191,5 @@
> +        yield File.makeDir(svFinalDirName, {ignoreExisting: true});
> +      }
> +
> +      let existsSvIndex = yield File.exists(OS.Path.join(svFinalDirName,
> +                                                   SINGLE_VARIANT_CONF_FILE));

nit: align SINGLE_VARIANT_CONF_FILE with OS.Path

@@ +200,5 @@
> +        debug("removing directory:" + svSourceDirName);
> +        File.removeDir(svSourceDirName, {
> +          ignoreAbsent: true,
> +          ignorePermissions: true
> +        });

don't you want to yield File.removeDir ?

Comment 32

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v3

r=fabrice

I've made all the requested changes except for this one:

>@@ +200,5 @@
>> +        debug("removing directory:" + svSourceDirName);
>> +        File.removeDir(svSourceDirName, {
>> +          ignoreAbsent: true,
>> +          ignorePermissions: true
>> +        });

> don't you want to yield File.removeDir ?

I have omitted "yield" in this line because we don't need a synchronous removal of the source dir

Comment 33

[Carmen Jimenez Cabezas]

Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=fe3925609083

Comment 34

[Carmen Jimenez Cabezas]

Try looks good, requesting checkin

Comment 35

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v3 B2G26_v1.2 version

r=fabrice

This is the b2g26 version. The other one is the mozilla central version and it doesn't apply directly to b2g26

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/67db0f727b94

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67db0f727b94

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/906c3568b844

Comment 39

[rafael.marquez]

Verified:
Device -> Unagi
Branch -> v1.2
Gecko -> 4df52ee
Gaia -> 8d762f3