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

VERIFIED FIXED in Firefox 28

Status

defect
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: rafael.marquez, Assigned: macajc)

Tracking

unspecified
1.2 C5(Nov22)
x86_64
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 verified, b2g-v1.2 verified)

Details

(Whiteboard: [systemsfe] )

Attachments

(2 attachments, 2 obsolete attachments)

****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"
    }
  ]
}
Blocks: 893807
blocking-b2g: --- → koi?
QA Contact: rafael.marquez
Whiteboard: [systemsfe]
Do we know what happens with 3rd party preinstalled apps in the old customization format in 1.1 when you do a factory reset?
Flags: needinfo?(rafael.marquez)
In version 1.1 there is no local 3rd party apps, it was added in version 1.2 for single variant
(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.
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.
(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.
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.
Okay, thanks for the clarification then. Removing needinfo then - sounds like we know we need to get the factory reset use case fixed.
Flags: needinfo?(rafael.marquez)
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.
(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?
Assignee: nobody → cjc
(In reply to Jason Smith [:jsmith] from comment #9)

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

Because /system is read-only directory
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.
blocking-b2g: koi? → -
(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.
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.
blocking-b2g: - → koi?
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?)
Flags: needinfo?(dbialer)
(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.
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.
Agree with Beatriz. All preloaded apps must remain after a factory reset.
Just to add one last comment - we also have contractual commitments with 3rd parties so there is a commercial implication here as well
Mentioned this in System FE Planning:

This needs a product call on what to do here.
Flags: needinfo?(pdolanjski)
Keywords: productwanted
Flags: needinfo?(dbialer)
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.
blocking-b2g: koi? → koi+
Flags: needinfo?(pdolanjski)
(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.
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.2 C5(Nov22)
Keywords: productwanted
Blocks: 939342
No longer blocks: 939342
Gregor,

Do we have an update here? Is SystemFE team looking at this?
Flags: needinfo?(anygregor)
Carmen,

What are the next steps here? What would be the ETA for the fix?
Flags: needinfo?(cjc)
Flags: needinfo?(anygregor) → needinfo?(marce)
Carmen already started to work on it, she will give more feedback today
Flags: needinfo?(marce)
Sorry for the delay, I going to upload a patch for this bug tomorrow.
Flags: needinfo?(cjc)
Posted patch Proposed patch v1 (obsolete) — Splinter Review
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.
Attachment #8336171 - Flags: review?(fabrice)
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
Attachment #8336171 - Flags: review?(fabrice)
Status: NEW → ASSIGNED
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.
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
Posted patch Proposed patch v2 (obsolete) — Splinter Review
changes requested
Attachment #8336171 - Attachment is obsolete: true
Attachment #8339262 - Flags: review?(fabrice)
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 ?
Attachment #8339262 - Flags: review?(fabrice) → review+
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
Attachment #8339262 - Attachment is obsolete: true
Attachment #8339589 - Flags: review+
Try looks good, requesting checkin
Keywords: checkin-needed
r=fabrice

This is the b2g26 version. The other one is the mozilla central version and it doesn't apply directly to b2g26
Attachment #8340818 - Flags: review+
Component: Gaia::Homescreen → DOM: Apps
Product: Firefox OS → Core
https://hg.mozilla.org/mozilla-central/rev/67db0f727b94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
Verified:
Device -> Unagi
Branch -> v1.2
Gecko -> 4df52ee
Gaia -> 8d762f3
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.