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

RESOLVED FIXED in mozilla5

Status

()

enhancement
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: KWierso, Assigned: jimm)

Tracking

Trunk
mozilla5
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 8 obsolete attachments)

Reporter

Description

9 years ago
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
Reporter

Updated

9 years ago
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.

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Assignee

Comment 2

9 years ago
(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.
Reporter

Comment 3

9 years ago
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.
Reporter

Comment 4

9 years ago
Posted patch first attempt (obsolete) — Splinter Review
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.
Attachment #477035 - Flags: feedback?(jmathies)
Reporter

Comment 5

9 years ago
Posted patch first attempt take two (obsolete) — Splinter Review
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.
Attachment #477035 - Attachment is obsolete: true
Attachment #477035 - Flags: feedback?(jmathies)
Assignee

Comment 6

9 years ago
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?
Reporter

Comment 8

9 years ago
Posted patch v3 w/nsis changes (obsolete) — Splinter Review
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.
Attachment #477036 - Attachment is obsolete: true
Attachment #477199 - Flags: feedback?(jmathies)
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?
Reporter

Comment 10

9 years ago
(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.)
Assignee

Comment 11

9 years ago
(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"); :)
Assignee

Comment 12

9 years ago
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.
Reporter

Comment 13

9 years ago
Posted patch v4 (obsolete) — Splinter Review
Something like this?
Attachment #477199 - Attachment is obsolete: true
Attachment #477199 - Flags: feedback?(jmathies)
Reporter

Comment 14

9 years ago
(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.
Reporter

Comment 15

9 years ago
Posted patch v5 (obsolete) — Splinter Review
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.
Attachment #477237 - Attachment is obsolete: true
Assignee

Comment 16

9 years ago
Posted patch patch (obsolete) — Splinter Review
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?
Attachment #477267 - Attachment is obsolete: true
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.
Assignee

Comment 18

9 years ago
Building a 64 bit build now, lets see if we can get this installer define sorted out.
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"
So yes, "#define HAVE_64BIT_OS" does the right thing
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.
Assignee

Comment 22

9 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #477287 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #477340 - Flags: review?(robert.bugzilla)
Assignee

Comment 24

9 years ago
Comment on attachment 477340 [details] [diff] [review]
patch

crap, _Win32 is also defined on 64 bit builds!
Attachment #477340 - Flags: review?(robert.bugzilla) → review-
Why don't you want to use / can't you use the HAVE_64BIT_OS define?
Assignee

Comment 26

9 years ago
Posted patch patch (obsolete) — Splinter Review
Again, using _X86_ in both places.
Attachment #477340 - Attachment is obsolete: true
Attachment #477346 - Flags: review?(robert.bugzilla)
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.
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.
Assignee

Comment 29

9 years ago
(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.
Assignee

Updated

9 years ago
Attachment #477346 - Flags: review?(robert.bugzilla)
(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 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.
Attachment #477346 - Flags: review-
Assignee

Comment 32

9 years ago
(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.
Assignee

Comment 33

9 years ago
Posted patch patchSplinter Review
Attachment #477346 - Attachment is obsolete: true
Attachment #477476 - Flags: review?(robert.bugzilla)
(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.
Assignee

Comment 35

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

Comment 36

8 years ago
Adding post-4.0 dependency since Win64 is on the roadmap for Firefox 5.
Depends on: post2.0
Assignee: nobody → jmathies
Assignee

Updated

8 years ago
Depends on: 577867
Jim, given the dependency that you just added, should this not be landing quite yet?
Whiteboard: fixed-in-cedar
Assignee

Comment 39

8 years ago
(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.
Assignee

Updated

8 years ago
No longer depends on: post2.0
http://hg.mozilla.org/mozilla-central/rev/611ba86e1171
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
Assignee

Comment 41

8 years ago
(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.
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
Assignee

Comment 43

8 years ago
(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
No longer depends on: 577867
Depends on: 577867
You need to log in before you can comment on or make changes to this bug.