Closed Bug 834515 Opened 11 years ago Closed 11 years ago

Updating to a packaged app with a new name via introducing a locale override allows changing the app name

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jsmith, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

Build: B2G 18 1/24/2013
Device: Unagi

Steps:

1. Install a packaged app with app name X and no locale override for the app name
2. Update the app on the server to the same app name X with a locale override with app name Y that matches the phone's locale
3. Check for updates
4. Apply the update

Expected:

The app name should be still be the old name from step #1 - app name X.

Actual:

The app name is the locale overridden app name - app name Y.
Blocks: 826555
At this point in the game we probably can't block on this. But we should probably try to get this in for v1.01.
tracking-b2g18: --- → ?
Ugh, this basically means that the changes in bug 826555 aren't working at all :( It's trivial to work around the checks there by playing around with locales. I think this is a blocker.
blocking-b2g: --- → tef?
Opps, looks like I made a mistake while addressing review comment from bug 826555.

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#262
  let defaultName = new ManifestHelper(aManifest, aApp.origin).name;
should be
  let defaultName = new ManifestHelper(previousManifest, aApp.origin).name;

But that doesn't explain this bug and bug 834518. It allows to rename the app with a new locale matching the os phone's locale, but the selected name will be the default new locale specified in manifest:
From: { name: 'X' }.
To:   { name: 'Z', { locale: { en: { name: 'Y' } } }.
The name will switch from X to Z.

I'll try to figure out what wrong while addressing that obvious error.
Assignee: nobody → poirot.alex
blocking-b2g: tef? → tef+
Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0
Attached patch WIP (obsolete) — Splinter Review
My unagi just died, so I wasn't able to finalize and test properly this patch.
But here is why I've tried to fix:
- In AppsUtils.normalizeManifest, aApp.manifest ends up being null. So that we should somehow retrieve the current app manifest (i.e. before the update). For that I modified checkForUpdate and factorize the call to _readManifest in order to be able to pass it to normalizeManifest.
- Instead of calling AppsUtils.normalizeManifest throught AppUtils.checkManifest in all scenarios: install, download and updates, I only call it from checkForUpdates. I ended up renaming this function to AppUtils.ensureSameAppName.

I'll try to borrow someone else phone while being at FOSDEM, but if anyone has free cycles, do not hesitate to take over this bug and the related one.
Attachment #708833 - Attachment is obsolete: true
Comment on attachment 709719 [details] [diff] [review]
Bug 834515 - Updating to a packaged app with a new name via introducing a locale override allows changing the app name r=fabrice

Desktop tests seems to pass but they crash at the end of test with a false assertion:
WARNING: NS_ENSURE_TRUE(!(exists)) failed: file /mnt/desktop/gecko/toolkit/profile/nsToolkitProfileService.cpp, line 650
^G###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file /mnt/desktop/gecko/xpcom/base/nsTraceRefcntImpl.cpp, line 441
NS_LogAddRef_P (/mnt/desktop/gecko/xpcom/base/nsTraceRefcntImpl.cpp:970)
nsAlertsService::AddRef() (/mnt/desktop/gecko/toolkit/system/gnome/nsAlertsService.cpp:10)
nsAlertsServiceConstructor (/mnt/desktop/gecko/toolkit/system/gnome/nsGnomeModule.cpp:26)
nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (/mnt/desktop/gecko/xpcom/components/nsComponentManager.cpp:1036)
nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (/mnt/desktop/gecko/xpcom/components/nsComponentManager.cpp:1427)
nsGetServiceByContractID::operator()(nsID const&, void**) const (/mnt/desktop/gecko/obj-x86_64-unknown-linux-gnu/xpcom/build/nsComponentManagerUtils.cpp:247)
nsCOMPtr<nsIAlertsService>::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) (/mnt/desktop/gecko/obj-x86_64-unknown-linux-gnu/toolkit/components/alerts/../../../dist/include/nsCOMPtr.h:1184)

I'm really not expecting to introduce such issue with this patch.


Fabrice, see comment 5 for more details on what I done in this patch.
Attachment #709719 - Flags: review?(fabrice)
Comment on attachment 709719 [details] [diff] [review]
Bug 834515 - Updating to a packaged app with a new name via introducing a locale override allows changing the app name r=fabrice

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

That looks good to me, but I need some time to run tests.
We're continuing to block on this due to the security impact.
Comment on attachment 709719 [details] [diff] [review]
Bug 834515 - Updating to a packaged app with a new name via introducing a locale override allows changing the app name r=fabrice

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

That looks good to me, but I need some time to run tests.
Attachment #709719 - Flags: review?(fabrice) → review+
Did the tests pass?  Does it look like this'll be landing soon?
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
https://hg.mozilla.org/mozilla-central/rev/519b528350ef
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Keywords: verifyme
QA Contact: jsmith
Tests:

- Rename "name" parameter on update (used packaged app) - PASS, but hit bug 834672 on the way
- Rename "name" parameter on locale override name on update - FAIL, bug 844369
- Add a locale override to override name to something different on update - PASS

Two followups:

- bug 844369
- bug 844243
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 844369
Depends on: 844243
No longer depends on: 844369
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: