597911 - Generate different application ID for 64-bit builds of Firefox.

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100919 Firefox/4.0b7pre
Build Identifier: 

I have both the 32-bit Firefox nightly and the 64-bit Firefox nightly installed.
Each has its own taskbar icon pinned to my Win7 taskbar. Each is set to use separate profiles, so they can be run simultaneously.

But when I start them both up, Windows deactivates the 64-bit taskbar icon, adding the 64-bit window to the 32-bit icon's preview.

Firefox should generate a different application ID for the two distributions, as explained in this bug's URL.


Alternatively, maybe this can be done on a profile-specific level, so each profile will open windows with a unique application ID.

Reproducible: Always

Comment 1

[Robert Strong (they/them - no direct email)]

I emailed jmathies a couple of weeks ago about supporting this use case and think it should be relatively simple to implement. Jim, do you think we should? Since we aren't releasing an Windows x64 build of Firefox 4.0 I do think we should hold off on this until after we branch for Firefox 4.0 though since there is plenty of other work to keep us busy.

Comment 2

[Jim Mathies [:jimm]]

(In reply to comment #1)
> I emailed jmathies a couple of weeks ago about supporting this use case and
> think it should be relatively simple to implement. Jim, do you think we should?
> Since we aren't releasing an Windows x64 build of Firefox 4.0 I do think we
> should hold off on this until after we branch for Firefox 4.0 though since
> there is plenty of other work to keep us busy.

Seems fine with me if someone has the time to implement it. I wouldn't block on this though as it's a pretty rare situation.

The code for app model id generation is here:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/WinTaskbar.cpp#272

and the code in the installer that sets / upgrades shortcuts created would also need updating.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I can't find anything exposed in nsIXULAppInfo interface that indicates 32 or 64 bit.

Available attributes are:
vendor - eg, "Mozilla"
name - eg, "Minefield"
ID - eg, some GUID"
version - "eg, "4.0b7pre"
appBuildID - this is just a timestamp from build time
platformVersion - this relates to the xulrunner/gecko platform, not the OS
platformBuildID - same here

The app model ID is currently generated from vendor, name, and version.

Would it need a new "architecture" attribute added to that interface, or is there somewhere else we can get the 64-bit-ness?

The only other thing I could think of would be to generate a completely distinct GUID for 64-bit Firefox, since then the ID attribute would be different. But then extensions would need to explicitly target both 32 and 64 bit versions of Firefox.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: first attempt

I have no idea if this even works as intended (or if it even builds, as apparently my build environment is broken for both 32 and 64-bit compilers...

Looking for feedback, though.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: first attempt take two

Ugh. Could've sworn I had wiped out all my previous changes before making the diff...

Same as before, but without an unrelated change in there.

Comment 6

[Jim Mathies [:jimm]]

We'll also need an update here:

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/defines.nsi.in

Rob, any hints on how to differentiate 32 bit from 64 bit defines in nsis?(In reply to comment #5)
> Created attachment 477036 [details] [diff] [review]
> first attempt take two
> 
> Ugh. Could've sworn I had wiped out all my previous changes before making the
> diff...
> 
> Same as before, but without an unrelated change in there.

I'd do an #ifdef #else to match the two so we have similar ids. 

We'll also need an update here:

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/defines.nsi.in

Rob, any hints on how to differentiate 32 bit and 64 bit defines in nsis?

Comment 7

[Robert Strong (they/them - no direct email)]

See the following
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/defines.nsi.in#41

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: v3 w/nsis changes

So apparently I don't know how to append a string to another string in C and still let it compile, so this patch has an ugly hack of appending one character at a time, just to get it to build. Hopefully someone can point out what I'm doing wrong so it doesn't do it like this.

This patch also includes what I think needs to be done for NSIS. I moved the !define ARCH stuff to the top of the file so it gets defined before AppUserModelID does, and then adds ARCH to the end of the ID.

Comment 9

[Robert Strong (they/them - no direct email)]

Hey Jim, I don't know if this is a possibility but if we could use an id that was generated from the install path we could avoid having to update shortcuts, etc. with each version change, etc. Is that possible?

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #8)
> Created attachment 477199 [details] [diff] [review]
> v3 w/nsis changes

I just tested a 32-bit build with this patch, and it does some weird things in the taskbar as Firefox is starting. (Profile Manager window uses a new taskbar icon, Firefox itself uses a new taskbar icon until it loads all the way.)

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #8)
> Created attachment 477199 [details] [diff] [review]
> v3 w/nsis changes
> 
> So apparently I don't know how to append a string to another string in C and
> still let it compile, so this patch has an ugly hack of appending one character
> at a time, just to get it to build. Hopefully someone can point out what I'm
> doing wrong so it doesn't do it like this.
> 
> This patch also includes what I think needs to be done for NSIS. I moved the
> !define ARCH stuff to the top of the file so it gets defined before
> AppUserModelID does, and then adds ARCH to the end of the ID.

Use AppendLiteral("abc"); :)

Comment 12

[Jim Mathies [:jimm]]

I would also suggest we move the aDefaultGroupId.IsEmpty() check up before the #ifdef, then append the architecture on the end and return PR_TRUE.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: v4

Something like this?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #13)
> Created attachment 477237 [details] [diff] [review]
> v4
> 
> Something like this?

And yes, I realize that I forgot the parentheses in that if statement.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: v5

Same as v4, but with the proper syntax for the if statement.

This seems to work correctly when I built it for 32-bit. For some reason, I'm getting c compiler errors when trying to build for 64-bit on my machine, so I can't tell if it does properly distinguish the 32/64-bit taskbar icons.

It DOES distinguish between this 32-bit taskbar icon and the official 32/64 bit nightly icons, so I'm assuming it is working.

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch

Here's a slight variation that builds.

Rob, is HAVE_64BIT_OS the right define to switch on in the installer? That doesn't appear to indicate which type of build we're making. Couldn't we use _Win64 here?

Comment 17

[Robert Strong (they/them - no direct email)]

I suspect it is just a bad name. I just verified that when building x86 on an x64 machine HAVE_64BIT_OS is not defined.

Comment 18

[Jim Mathies [:jimm]]

Building a 64 bit build now, lets see if we can get this installer define sorted out.

Comment 19

[Robert Strong (they/them - no direct email)]

btw: I also have an x64 build... the relevant part of the preprocessed defines.nsi.in after it has been preprocessed is below:
!define HAVE_64BIT_OS
!define ARCH "x64"
!define MinSupportedVer "Microsoft Windows Vista x64"
The same from my x86 build
!define ARCH "x86"
!define MinSupportedVer "Microsoft Windows 2000"

Comment 20

[Robert Strong (they/them - no direct email)]

So yes, "#define HAVE_64BIT_OS" does the right thing

Comment 21

[Robert Strong (they/them - no direct email)]

I also just read configure.in and verified that HAVE_64BIT_OS is for the compile environment and just to be sure asked khuey and he also verified it.

Comment 22

[Jim Mathies [:jimm]]

Attachment: patch

Man, finding suitable defines to key off of was a total pita. In widget code, _WIN32 and _WIN64 work fine. But for the defines, we don't have _WIN32, but we do have _WIN64. So I ended up using _X86_, which according to configure gets set when you're building a 32-bit build and targeting x86 hardware.

Comment 23

[Jim Mathies [:jimm]]

Here's the config parts:

http://mxr.mozilla.org/mozilla-central/source/configure.in#2488

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 477340 [details] [diff] [review]
patch

crap, _Win32 is also defined on 64 bit builds!

Comment 25

[Robert Strong (they/them - no direct email)]

Why don't you want to use / can't you use the HAVE_64BIT_OS define?

Comment 26

[Jim Mathies [:jimm]]

Attachment: patch

Again, using _X86_ in both places.

Comment 27

[Robert Strong (they/them - no direct email)]

So, _WIN64 is dependent on HAVE_64BIT_OS

The patch has a fallback I really don't like. We should just depend on whether or not HAVE_64BIT_OS or alternatively _WIN64 is defined.

If you don't like the HAVE_64BIT_OS just add a comment that it means it is an x64 compiler environment.

Comment 28

[Robert Strong (they/them - no direct email)]

also, I have some work that blocks Firefox 4.0 while this bug is for Windows x64 which we aren't releasing for Firefox 4.0 so this will have to wait.

Comment 29

[Jim Mathies [:jimm]]

(In reply to comment #27)
> So, _WIN64 is dependent on HAVE_64BIT_OS
> 
> The patch has a fallback I really don't like. We should just depend on whether
> or not HAVE_64BIT_OS or alternatively _WIN64 is defined.
> 
> If you don't like the HAVE_64BIT_OS just add a comment that it means it is an
> x64 compiler environment.

I'd prefer _WIN64 since HAVE_64BIT_OS is so horribly named. Don't we need a fallback or are we assured it will always be one or the other? Actually, I have a better idea, see below..

(In reply to comment #28)
> also, I have some work that blocks Firefox 4.0 while this bug is for Windows
> x64 which we aren't releasing for Firefox 4.0 so this will have to wait.

So we don't want to change the id post a release since it'll cause pinned taskbar and start menu links to break. So how about we skip Win32 all together and just tag on .Win64 for 64-bit builds? That way we don't have to get this in and approved for 4.0.

Comment 30

[Robert Strong (they/them - no direct email)]

(In reply to comment #29)
> (In reply to comment #27)
> > So, _WIN64 is dependent on HAVE_64BIT_OS
> > 
> > The patch has a fallback I really don't like. We should just depend on whether
> > or not HAVE_64BIT_OS or alternatively _WIN64 is defined.
> > 
> > If you don't like the HAVE_64BIT_OS just add a comment that it means it is an
> > x64 compiler environment.
> 
> I'd prefer _WIN64 since HAVE_64BIT_OS is so horribly named.
That's fine but I'd like the other ifdef changed then... no matter how horribly named using _WIN64 and HAVE_64BIT_OS in the same code for the same thing is horribly worse than that name!

> Don't we need a
> fallback or are we assured it will always be one or the other?
What else could it be besides x86 or x64?

> Actually, I have
> a better idea, see below..
> 
> (In reply to comment #28)
> > also, I have some work that blocks Firefox 4.0 while this bug is for Windows
> > x64 which we aren't releasing for Firefox 4.0 so this will have to wait.
> 
> So we don't want to change the id post a release since it'll cause pinned
> taskbar and start menu links to break. So how about we skip Win32 all together
> and just tag on .Win64 for 64-bit builds? That way we don't have to get this in
> and approved for 4.0.
These get set on every app update and install so I'm not sure what yoou are concerned about besides possibly the case where a nightly user is running Firefox with one account and updating it with a different one.

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 477346 [details] [diff] [review]
patch

Change all occurrences in the installer code from HAVE_64BIT_OS to _WIN64 including the NSIS !define HAVE_64BIT_OS and the places it is used... it needs to be one or the other.

Comment 32

[Jim Mathies [:jimm]]

(In reply to comment #31)
> Comment on attachment 477346 [details] [diff] [review]
> patch
> 
> Change all occurrences in the installer code from HAVE_64BIT_OS to _WIN64
> including the NSIS !define HAVE_64BIT_OS and the places it is used... it needs
> to be one or the other.

I'll just use HAVE_64BIT_OS then.

Comment 33

[Jim Mathies [:jimm]]

Attachment: patch

Comment 34

[Robert Strong (they/them - no direct email)]

(In reply to comment #32)
> (In reply to comment #31)
> > Comment on attachment 477346 [details] [diff] [review] [details]
> > patch
> > 
> > Change all occurrences in the installer code from HAVE_64BIT_OS to _WIN64
> > including the NSIS !define HAVE_64BIT_OS and the places it is used... it needs
> > to be one or the other.
> 
> I'll just use HAVE_64BIT_OS then.
I'm fine either way but thanks... I definitely want the ability to search on one define for x64 the bits in the files otherwise it would be entirely to easy to miss.

Comment 35

[Jim Mathies [:jimm]]

I filed a follow up (bug 598615) on potentially getting rid of or renaming that. We'll see where that goes.

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Adding post-4.0 dependency since Win64 is on the roadmap for Firefox 5.

Comment 37

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/611ba86e1171

Comment 38

[Boris Zbarsky [:bzbarsky]]

Jim, given the dependency that you just added, should this not be landing quite yet?

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> Jim, given the dependency that you just added, should this not be landing quite
> yet?

Yeah, lets not land this on mc. Sorry, I should have killed this patch. I'm pretty sure we can get bug 577867 done by the time we support 64bit.

Comment 40

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/611ba86e1171

Comment 41

[Jim Mathies [:jimm]]

(In reply to comment #40)
> http://hg.mozilla.org/mozilla-central/rev/611ba86e1171

Oh well. :) no harm done, this is an ok temp fix for 64 bit builds.

Comment 42

[Makoto Kato [:m_kato]]

Jim, this causes bugstage on Win64.  You shouldn't use "//" as comment.


 - ShellLink::SetShortCutShowMode
 - ShellLink::SetShortCutTarget
 - ShellLink::SetShortCutWorkingDirectory
 - UAC::Exec
 - UAC::ExecCodeSegment
 - UAC::ExecWait
 - UAC::GetElevationType
 - UAC::GetOuterHwnd
 - UAC::GetShellFolderPath
 - UAC::IsAdmin
 - UAC::RunElevated
 - UAC::ShellExec
 - UAC::ShellExecWait
 - UAC::StackPush
 - UAC::SupportsUAC
 - UAC::Unload
Invalid command: //
!include: error in script: "defines.nsi" on line 6
Error in script "uninstaller.nsi" on line 81 -- aborting creation process
make[6]: *** [uninstaller] Error 1

Comment 43

[Jim Mathies [:jimm]]

(In reply to comment #42)
> Jim, this causes bugstage on Win64.  You shouldn't use "//" as comment.
> 
> 
>  - ShellLink::SetShortCutShowMode
>  - ShellLink::SetShortCutTarget
>  - ShellLink::SetShortCutWorkingDirectory
>  - UAC::Exec
>  - UAC::ExecCodeSegment
>  - UAC::ExecWait
>  - UAC::GetElevationType
>  - UAC::GetOuterHwnd
>  - UAC::GetShellFolderPath
>  - UAC::IsAdmin
>  - UAC::RunElevated
>  - UAC::ShellExec
>  - UAC::ShellExecWait
>  - UAC::StackPush
>  - UAC::SupportsUAC
>  - UAC::Unload
> Invalid command: //
> !include: error in script: "defines.nsi" on line 6
> Error in script "uninstaller.nsi" on line 81 -- aborting creation process
> make[6]: *** [uninstaller] Error 1

http://hg.mozilla.org/mozilla-central/rev/8e9eff6c5e69