Firefox installer writes incorrect AppID value (with a leading space) to the Windows 7 registry resulting in double taskbar icons

VERIFIED FIXED in Firefox 18

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: sidrabbit, Assigned: bbondy)

Tracking

(Blocks 1 bug)

Trunk
Firefox 19
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

As per bug 577867, when I install Firefox on a Windows 7 system, the installer computes its AppID based on the installation path hash. On my 32-bit Windows 7 it's "A3710B8EBB50CD3" with default path. A shortcut with the same AppID is pinned to the taskbar by default. The value stored in the registry, however, is " A3710B8EBB50CD3" (note the leading space). That doesn't match the AppID of pinned shortcut, resulting in double taskbar entity when I start Firefox from this shortcut.

I got this while installing Firefox on the first time on a clean Windows 7 machine.
Blocks: 577867
Jim, I haven't investigated too much as to why but CityHash is returning a leading space for a path of "C:\Program Files\Nightly". I'm tempted to just remove the space since it is remove when adding the app model id to shortcuts. Thoughts?
Target Milestone: --- → Firefox 13
Version: Trunk → 14 Branch
Since we don't know what other paths can return a leading space, may be we should simply trim spaces from _every_ CityHash output?
Are we sure this is CH output and not a bug in the nsis plugin or installer script? (I'm fine with stripping whitespace though, I don't see any issues with that.)
(In reply to Sid from comment #2)
> Since we don't know what other paths can return a leading space, may be we
> should simply trim spaces from _every_ CityHash output?
Though I didn't state it explicitly, that is what we would do.

(In reply to Jim Mathies [:jimm] from comment #3)
> Are we sure this is CH output and not a bug in the nsis plugin or installer
> script? (I'm fine with stripping whitespace though, I don't see any issues
> with that.)
I haven't investigated this all that much but it definitely is not the script... might be the plugin though.
No longer blocks: 577867
The "double taskbar entity" has been reported at random since 27 May 2012 on mozillaZine Nightly build thread[1].

Confirming and updating version accordingly.
 
1 | http://forums.mozillazine.org/viewtopic.php?p=12015611&sid=d68f41cefe9a1bd81e3140a58d95921c#p12015611
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Summary: Firefox installer writes incorrect AppID value (with a leading space) to the Windows 7 registry → Firefox installer writes incorrect AppID value (with a leading space) to the Windows 7 registry resulting in double taskbar icons
Version: 14 Branch → Trunk
Nightly (Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120627030527) does the double taskbar icon after every update.

firefox.exe also loses its "Description" on the "Windows Task Manager" after each update, is this the same bug or a different one?
Posted patch cityhash patch (obsolete) — Splinter Review
Bug 768378 looks like a dupe of this one. The string should be ok with that space, assuming throughout the install process and in pulling the string from the registry in widget the space is respected as part of the id. But if for some reason we or Windows trim this off in handling, it can break things, so we might as well get rid of it.
Assignee: nobody → jmathies
Attachment #637109 - Flags: review?(robert.bugzilla)
Posted patch cityhash patch (obsolete) — Splinter Review
argh - windows line endings stripped.
Attachment #637109 - Attachment is obsolete: true
Attachment #637109 - Flags: review?(robert.bugzilla)
Attachment #637111 - Flags: review?(robert.bugzilla)
Blocks: 768378
Comment on attachment 637111 [details] [diff] [review]
cityhash patch

putting this on hold, it may not be needed.
Attachment #637111 - Flags: review?(robert.bugzilla)
(In reply to Jim Mathies [:jimm] from comment #9)
> Comment on attachment 637111 [details] [diff] [review]
> cityhash patch
> 
> putting this on hold, it may not be needed.

I'd say it's needed as Nightly creates a duplicate task bar icon on every update in Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120802030533
No longer blocks: 768378
Depends on: 768378
Blocks: 798805, 798375
Assignee: jmathies → netzen
The leading space on the appusermodelid is what turned out to break Win8 x86 Metro registration
So the issue here with CityHash is this line of code:
>  swprintf_s(hexResult, 17, L"%16I64X", result);


It will prefix a space because it requires 16 characters.  But sometimes a hash is only 15 hex characters.  So we can just prefix the 16 with a 0 to pad with 0s.  Or not have a minimum.
Good catch! I think going with a 15 char hash would be more appropriate.
Attachment #637111 - Attachment is obsolete: true
Attachment #674361 - Flags: review?(jmathies)
This plugin was built the same way as the previous binary was, with VS2008 (v9).
It generates a dll which is the exact same size as well.
Attachment #674363 - Flags: review?(jmathies)
Changes to NSIS to always overwrite the appusermodelid in the registry with the new value calculated.
Attachment #674365 - Flags: review?(jmathies)
Same as before but built with vc6. It is 13KB less than the original dll.
Attachment #674363 - Attachment is obsolete: true
Attachment #674363 - Flags: review?(jmathies)
Attachment #674418 - Flags: review?(jmathies)
Attachment #674418 - Attachment description: Patch v1 - CityHash plugin dll - built with vc6 → Patch v2 - CityHash plugin dll - built with vc6
No longer require 16 chars. If it is less it will use less chars in the output buffer.  Also converted the project so it can be built with vc6.  This is desirable for smaller file sizes.
Attachment #674361 - Attachment is obsolete: true
Attachment #674361 - Flags: review?(jmathies)
Attachment #674421 - Flags: review?(jmathies)
Attachment #674365 - Flags: review?(jmathies) → review+
Comment on attachment 674421 [details] [diff] [review]
Patch v2 - CityHash plugin fix

>diff --git a/other-licenses/nsis/Contrib/CityHash/CityHash.cpp b/other-licenses/nsis/Contrib/CityHash/CityHash.cpp
>--- a/other-licenses/nsis/Contrib/CityHash/CityHash.cpp
>+++ b/other-licenses/nsis/Contrib/CityHash/CityHash.cpp
>@@ -51,30 +51,31 @@ void pushString(const TCHAR *str)
>   lstrcpyn(th->text, str, strLen);
>   th->next = *g_stacktop;
>   *g_stacktop = th;
> }
> 
> extern "C"
> {
> 
>-CITYHASH_API
> void GetCityHash64(HWND hwndParent, int string_size, char *variables, stack_t **stacktop)
> {
>   TCHAR hashString[MAX_STRLEN];
>   TCHAR hexResult[18];
> 
>   g_stacktop = stacktop;
>   g_variables = variables;
> 
>   memset(hashString, 0, sizeof(hashString));
>   memset(hexResult, 0, sizeof(hexResult));
> 
>   if (!popString(hashString)) {
>     pushString(L"error");
>     return;
>   }
>   uint64 result = CityHash64((const char*)&hashString[0], wcslen(hashString)*sizeof(TCHAR));
>-  swprintf_s(hexResult, 17, L"%16I64X", result);
>+  // If the hash happens to work out to less than 16 hash digits it will be
>+  // prefixed with zeros.
Per (In reply to Brian R. Bondy [:bbondy] from comment #18)
> Created attachment 674421 [details] [diff] [review]
> Patch v2 - CityHash plugin fix
> 
> No longer require 16 chars. If it is less it will use less chars in the
> output buffer.  Also converted the project so it can be built with vc6. 
> This is desirable for smaller file sizes.

>+  swprintf(hexResult, L"%I64X", result);
Attachment #674421 - Flags: feedback+
Had an old version of one of the files in the last patch which had the old comment about prefixing 0's. Updated to the new file which has the comment about using less of the buffer instead.
Attachment #674421 - Attachment is obsolete: true
Attachment #674421 - Flags: review?(jmathies)
Attachment #674568 - Flags: review?(jmathies)
Attachment #674418 - Flags: review?(jmathies) → review+
Attachment #674568 - Flags: review?(jmathies) → review+
Confirmed no duplicate taskbar icon after today's update to Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121026030606
Status: RESOLVED → VERIFIED
Duplicate of this bug: 798805
Duplicate of this bug: 802814
Depends on: 806977
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 577867
User impact if declined: Users can have double taskbar icons if they install an x86 build on x86 Windows.  The hash in that case works out to less than 16 characters which creates a second taskbar icon.
Testing completed (on m-c, etc.): This has been on m-c for a bit.
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: none.
Attachment #677229 - Flags: approval-mozilla-beta?
Attachment #677229 - Flags: approval-mozilla-aurora?
Comment on attachment 677229 [details] [diff] [review]
Patch v1 - Consolidated patch for Beta/Aurora

Approving uplift to Aurora to go along with bug 768378, too late for Beta.
Attachment #677229 - Flags: approval-mozilla-beta?
Attachment #677229 - Flags: approval-mozilla-beta-
Attachment #677229 - Flags: approval-mozilla-aurora?
Attachment #677229 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.