764172 - [Win & Linux] Reinstalling an App that was previously launched deletes an old profile and re-creates a different app profile

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Jason Smith [:jsmith]]

Steps for Windows and Linux:

1. Go to apps.mozillalabs.com/appdir
2. Install Mozilla QA WebRT Tester
3. Launch the app
4. Go to firefox --> geolocation --> position.html
5. Select remember my choice and make a selection
6. Close the app
7. Reinstall the app
8. Launch the app
9. Go to firefox --> geolocation --> position.html

Expected:

The choice remembered should still be remembered and reused for geolocation.

Actual:

A prompt appears again asking for the user to share their location - the choice made originally that was remembered was lost.

Additional Notes:

Only appears to happen on Windows & Linux. Works correctly on Mac.

Comment 1

[Marco Castelluccio [:marco]]

This is most probably caused by Firefox removing the profile directory on apps re-installation (only on Win and Linux).

Comment 2

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio from comment #1)
> This is most probably caused by Firefox removing the profile directory on
> apps re-installation (only on Win and Linux).

Confirmed on Windows. The profile directory is indeed wiped on a reinstall of a web application previously launched.

Comment 3

[Jason Smith [:jsmith]]

k9o nomination - we shouldn't be wiping profile data on a reinstall - otherwise we lose anything that's tied to an app profile.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Why in _createConfigFiles (for Win) there are:
> writer = null;
> factory = null;
Do we need to explicitly set them to null?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 633793 [details] [diff] [review]
Patch v2

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

r- for the for..of fix and hoping that we can do the refactor suggested, but everything looks fine

::: browser/modules/WebappsInstaller.jsm
@@ +264,5 @@
> +                           this.configJson,
> +                           this.webappINI
> +                          ];
> +
> +      for(let file in filesToRemove) {

this doesn't work, you need to use for..of instead of for..in to iterate through the array contents instead of the keys.

@@ +295,3 @@
>          this.startMenuShortcut.remove(false);
>        }
> +    } catch(ex) {}

I wasn't gonna request this in this bug, but since there's the for..of thing to fix, I believe we should do it too:

I think we can make this whole section a bit cleaner by doing:

```
let filesToRemove = [this.desktopShortcut, this.startMenuShortcut];

if (keepProfile) {
  filesToRemove.append(this.iconFile);
  filesToRemove.append(...);
} else {
  // Remove everything
  filesToRemove.append(this.installDir);
}

... for loop to remove files ...
```

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 8

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8be9af9183

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea8be9af9183

Comment 10

[Jason Smith [:jsmith]]

Looks good on Windows.

Comment 11

[Jason Smith [:jsmith]]

Looks good on Linux. Verified.