1049521 - Be less aggressive about non-primary file type associations in Firefox on Windows

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Mike Kaply [:mkaply]]

We've received two enterprise reports so far of Firefox taking the PDF association from Adobe Reader and I've had it happen to me as well.

This was when Firefox 31 ESR was installed over Firefox 24 ESR.

Unfortunately we're still trying to get a recreation scenario.

There really should be an install switch to prevent this to happen.

Also, this was a very bold move to make considering that our PDF reader is not up to par with Acrobat Reader (particularly with printing).

Comment 1

[Mike Kaply [:mkaply]]

On my machine right now, PDF is set to FirefoxHTML even though I've always had PDF installed and it has always been the default.

Comment 2

[Mike Kaply [:mkaply]]

I think I know what's going on here.

Adobe Reader installs at the system level and sets:

HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.pdf is AcroExch.Document11

Firefox installs at the user level and checks SHCTX in the installer which for Firefox will be HKEY_CURRENT_USER\Software\Classes\.pdf

It's not set in the PDF case, so Firefox takes it.

Firefox should not be checking SHCTX, it should be checking HKLM and HKCU explicitly to see if .pdf is associated in either place and if so, do nothing.

Comment 3

[Mike Kaply [:mkaply]]

I just verified my scenario and I was correct.

I cleaned out all .pdf from my registry.

I installed Acrobat Reader.

The only .pdf association was 

HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.pdf with AcroExch.Document11

HKEY_CLASSES_ROOT\.pdf was AcroExch.Document11 as well.

After installing Firefox 31 and allowing Firefox to become the default browser, I know have

HKEY_CURRENT_USER\Software\Classes\.pdf as FirefoxHTML and HKEY_CLASSES_ROOT\.pdf is FirefoxHTML as well.

Comment 4

[Charles]

Interesting... thanks for mailing this one Mike.

I can confirm the bug, and indeed, it is very presumptuous of Firefox to say the least... ;)

Even if the Firefox native PDF renderer was equal to Adobe Reader, it still should never do this.

Hopefully a fix will be forthcoming... but for now, I just have to remember to check this and change it back. Weirdly, it doesn't appear to always happen. Not sure why...

Comment 5

[Charles]

mailing should = nailing

Comment 6

[hartnegg]

I regard this a serious bug, because it appears to break all installs of Adobe Reader.

Didn't Mozilla announce that this would only happen if no other pdf-viewer is installed? Now it does happen even with Adobe Reader installed.

But even if it would only affect machines with no pdf viewer previously installed, the pdfs are shown in Windows Explorer as having Filetype "Firefox HTML document". This is always wrong.

As update to comment 1: it does not only happen when updating from 24 to 31, but also when installing 31 on a machine with no Firefox installed before.

Comment 7

[hartnegg]

Workaround:
Do not allow Firefox to set itself as default browser during install.
Run "helper /SetAsDefaultAppGlobal" in the uninstall directory.
Set the preference "browser.shell.checkDefaultBrowser" to false.

Comment 8

[Charles]

(In reply to hartnegg from comment #6)
> I regard this a serious bug, because it appears to break all installs of
> Adobe Reader.

Well.... not precisely true. It dissacociates PDFs from Adobe Reader, yes, but it certainly doesn't 'break' the install.

All you have to do is re-associate PDFs - right-clickon a PDF > 'Open With' > 'Choose default program', select Adobe Reader and be sure the 'always...' checkbox is checked.

It is extremely annoying though...

Comment 9

[Charles]

(In reply to hartnegg from comment #7)
> Workaround:
> Do not allow Firefox to set itself as default browser during install.
> Run "helper /SetAsDefaultAppGlobal" in the uninstall directory.
> Set the preference "browser.shell.checkDefaultBrowser" to false.

And how do you do this is during a clean install? Or if your method for updating workstations is to first uninstall the old version then install the new one?

Comment 10

[Vladan Djeric (:vladan)]

Was this issue caused by bug 452254?

Comment 11

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

Yes and we'll look into this as soon as the Mac vs signing is done.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: patch rev1.

Ideally we'd pass in the root key to the macro, but there's no need to over complicate until we need that functionality.
I was able to reproduce only by staging my registry.

Comment 14

[Mike Kaply [:mkaply]]

Can we please just not take over PDF? It's pretty presumptuous of us to take PDF viewing.

Comment 15

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

This will only take over pdf if there is nothing already handling pdf.

Comment 16

[Charles]

Firefox is a WEB BROWSER.

It should NEVER take over FILESYSTEM level file associations for PDFs.

Comment 17

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

Comment on attachment 8488714 [details] [diff] [review]
patch rev1.

Please fix the comment  "s/SHCTX/HKCR/ is the root key to search". I think it might be a good thing to also check if HKLM or HKCU has a value other than FirefoxHTML then if either HKLM or HKCU has a value of FirefoxHTML clear it. What do you think?

Comment 18

[Brian R. Bondy [:bbondy]]

I actually think that adobe is in the wrong here for not changing HKCU and only changing HKLM. But I think the checks we're doing in this new patch is sufficient because we don't care too much about the defaults. 

So what you're suggesting is check HKLM software/classes directly and compare it to HKCU software/classes and if they are different and HKCU softwrae/classes is FirefoxHTML then clear HKCU?

Comment 19

[Brian R. Bondy [:bbondy]]

(In reply to Mike Kaply (:mkaply) from comment #14)
> Can we please just not take over PDF? It's pretty presumptuous of us to take
> PDF viewing.

This patch will not `take` PDF. There was a bug that made that happen with previous versions, but that was a bug, and this is the task we're fixing it in.

(In reply to Charles from comment #16)
> Firefox is a WEB BROWSER.

Indeed.
 
> It should NEVER take over FILESYSTEM level file associations for PDFs.

Agreed, there is no argument here.

Comment 20

[Mike Kaply [:mkaply]]

> This patch will not `take` PDF. There was a bug that made that happen with previous versions, but that was a bug, and this is the task we're fixing it in.

Even this bug fixed, Firefox will associate with PDFs if there is no PDF reader installed, correct?

Comment 21

[Brian R. Bondy [:bbondy]]

yes, although only if there is nothing else on the system. It's a format that we can read though, so I'm not sure why that's a problem.

Comment 22

[Dave Lewis]

No problem if there is nothing that can read plus. However I have made sure that my enterprise can read pdfs through adobe reader and in special cases acrobat. I shouldn't have to rectify that after I provide my staff with a Web browser regardless of whether it can handle it or not.

Comment 23

[Mike Kaply [:mkaply]]

> yes, although only if there is nothing else on the system. It's a format that we can read though, so I'm not sure why that's a problem.

Because Firefox is a web browser. It's not a PDF viewer.

You just agreed to this argument:

>> It should NEVER take over FILESYSTEM level file associations for PDFs.
>Agreed, there is no argument here.

So why would Firefox ever take over PDFs? It doesn't make sense.

Comment 24

[Brian R. Bondy [:bbondy]]

I was agreeing to `taking over` and not to associating when there was nothing else provided.

Comment 25

[hartnegg]

At least the labeling of the files is wrong. They are shown in Windows Explorer as type "Firefox HTML Document".

PDF is definitely not HTML.

Also why "Document", I installed the german version, it should be "Dokument".

Comment 26

[Brian R. Bondy [:bbondy]]

I'll defer to rstrong on whether he wants pdf associations completely removed or not.
In which case we should re-open "Bug 1064390 - DO NOT associate *.pdf with Firefox" and do this bug for the other file types that we still do want.

But I do want to comment that this bug is making the association problem less severe than it is today.
It is not the place that Mozilla is introducing such associations, and it is not meant to cover all bugs surrounding PDF association such as it being listed as HTML document.  

These are all valid concerns, but the patches in this bug have no need to be held up by anything mentioned so far.

Comment 27

[Charles]

I repeat - Firefox is a WEB BROWSER.

It should NEVER, EVER take over ANY OS level file associations for binary files.

Even if he day ever comes when it can render more than 50% of the PDFs we routinely use (to date it simply chokes on them more often than not), I would still be against it ever taking over the OS level association.

Comment 28

[Brian R. Bondy [:bbondy]]

Please continue any discussion about us not handling PDF at all, even in the absence of a PDF viewer in bug 1064390.

Comment 29

[Masatoshi Kimura [:emk]]

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> I actually think that adobe is in the wrong here for not changing HKCU and
> only changing HKLM.

How Adobe can change HKCU in the following case?
1. The user installed Adobe Reader.
2. The user created a new Windows account and logged on the account.
3. The user installed Firefox.
We should not assume HKCU is always set.

Comment 30

[Jim Mathies [:jimm]]

(In reply to Mike Kaply (:mkaply) from comment #14)
> Can we please just not take over PDF? It's pretty presumptuous of us to take
> PDF viewing.

Why? I think we should place ourselves on even footing with any other application that can render pdf.

Comment 31

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

Please take any discussion as to whether we should or should not handle local pdf files when there is currently no handler for pdf files over to bug 1064390.

Comment 32

[Masatoshi Kimura [:emk]]

Hey, bug 1064390 is duplicated to THIS bug. If you would not like to hear any opinions, please say so.

Comment 33

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

(In reply to Masatoshi Kimura [:emk] from comment #32)
> Hey, bug 1064390 is duplicated to THIS bug. If you would not like to hear
> any opinions, please say so.
That is an assumption on your part. As a matter of fact when I discussed this with bsmedberg I took the position that we should remove it. I will undupe that bug and wontfix it so that assumption won't be made again. Please take further discussion over to bug 1064390.

Comment 34

[Charles]

(In reply to Jim Mathies [:jimm] from comment #30)
> (In reply to Mike Kaply (:mkaply) from comment #14)
> > Can we please just not take over PDF? It's pretty presumptuous of us to take
> > PDF viewing.
> 
> Why? I think we should place ourselves on even footing with any other
> application that can render pdf.

I'll say it again.

When Firefox can render PDFs reasonably well, without crashing the browser or bringinig it to its knees, AND can then also PRINT them without lots of problems (visual as well as performance), then you might at least have half a leg to stand on.

As it stands, it is the worst PDF renderer I have ever seen, bar none, and the presumptuousness being vocalized here is staggering.

LEAVE MY FILE ASSOCIATIONS ALONE.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Restrict associations more to be set in less cases than currently

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: Fix people affected by the bug - remove already set association when there's an HKLM association

Note that this part of the fix wasn't added inside the existing association check because that's only run when defaults are being set. 
So it was needed to have a new macro and it was needed to call it from PostUpdate to fix those affected.

Comment 37

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

Comment on attachment 8489050 [details] [diff] [review]
Restrict associations more to be set in less cases than currently

># HG changeset patch
># Parent 74a40441372aa8e51765b74024368481eeda4334
># User Brian R. Bondy <[e-mail]>
>Bug 1049521 - Only register for types when there is no default in either of HKLM or HKCU. r=rstrong
>
>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -426,41 +426,41 @@ FunctionEnd
>   ${EndIf}
> 
>   ReadRegStr $6 SHCTX "$0\.xhtml" ""
>   ${If} "$6" != "FirefoxHTML"
>     WriteRegStr SHCTX "$0\.xhtml" "" "FirefoxHTML"
>   ${EndIf}
> 
>   ; Only add .oga if it's not present
>-  ${CheckIfRegistryKeyExists} "$0" ".oga" $7
>+  ${CheckIfRegistryKeyExists} "" ".oga" $7
>   ${If} $7 == "false"
>     WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
>   ${EndIf}
There is a much simpler and more efficient way to check than what the CheckIfRegistryKeyExists macro uses. I'd prefer to just get rid of it after Seamonkey no longer uses it and use the following.
ClearErrors
EnumRegKey $7 HKCR ".ogv" 0
${If} ${Errors}
  WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
${EndIf}

r=me with that change

Comment 38

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

Comment on attachment 8489051 [details] [diff] [review]
Fix people affected by the bug - remove already set association when there's an HKLM association

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -51,16 +51,39 @@ Function RegisterStartMenuTile
>       ${If} "$AppUserModelID" != ""
>         ApplicationID::Set "$SMPROGRAMS\${BrandFullName}.lnk" "$AppUserModelID" "true"
>       ${EndIf}
>     ${EndIf}
>   ${EndIf}
> !endif
> FunctionEnd
> 
>+; Due to a bug with file associations, we were only checking SHCTX for some
>+; formats like .pdf.  SHCTX is set to HKCU or HKLM depending on if it's
>+; running elevated.  The bug would happen when we would check HCKU which
>+; wouldn't exist because some programs only set the HKLM Software/Classes keys
>+; for default association.  The fix was to use the merged view HKCR to see
>+; existance of an existing association. To clean installations affected by
>+; this, we use this macro which will remove HKCU associations of FirefoxHTML
>+; if HKLM is set and different from HKCU for the specified keys.

; Due to a bug when associating some file handlers, only SHCTX was checked for
; some file types such as ".pdf". SHCTX is set to HKCU or HKLM depending on
; whether the installer has write access to HKLM. The bug would happen when
; HCKU was checked and didn't exist since programs aren't required to set the
; HKCU Software\Classes keys when associating handlers. The fix uses the merged
; view in HKCR to check for existance of an existing association. This macro
; cleans affected installations by removing the HKLM and HKCU value if it is set
; to FirefoxHTML when there is a value for PersistentHandler or by removing the
; HKCU value when the HKLM value has a value other than an empty string.

>+!macro FixBadFileAssociation FILE_TYPE
>+    ReadRegStr $0 HKCU "Software\Classes\${FILE_TYPE}" ""
>+    ReadRegStr $1 HKLM "Software\Classes\${FILE_TYPE}" ""
>+    ; If HKCU is set to Firefox
>+    ${If} $0 == "FirefoxHTML"
>+    ; And HKLM is not
>+    ${AndIf} $1 != "FirefoxHTML"
>+    ; And only if HKLM actually specifies something
>+    ${AndIf} $1 != ""
>+      ; Reset the file association
>+      DeleteRegKey HKCU "Software\Classes\${FILE_TYPE}"
>+    ${EndIf}
>+!macroend
>+!define FixBadFileAssociation "!insertmacro FixBadFileAssociation"
It is possible to have values for PersistentHandler, OpenWithList, or OpenWithProgids for a file handler and have an empty default value and this as it stands would remove those as well. Perhaps something like this?

  ; Only delete the default value in case the key has values for OpenWithList,
  ; OpenWithProgids, PersistentHandler, etc.
  ReadRegStr $0 HKCU "Software\Classes\${FILE_TYPE}" ""
  ReadRegStr $1 HKLM "Software\Classes\${FILE_TYPE}" ""
  ReadRegStr $2 HKCR "${FILE_TYPE}\PersistentHandler" ""
  ${If} "$2" != ""
    ; Since there is a persistent handler remove FirefoxHTML as the default
    ; value from both HKCU and HKLM if it set to FirefoxHTML.
    ${If} "$0" == "FirefoxHTML"
      DeleteRegValue HKCU "Software\Classes\${FILE_TYPE}" ""
    ${EndIf}
    ${If} "$1" == "FirefoxHTML"
      DeleteRegValue HKLM "Software\Classes\${FILE_TYPE}" ""
    ${EndIf}
  ${ElseIf} "$0" == "FirefoxHTML"
    ; Since KHCU is set to FirefoxHTML remove FirefoxHTML as the default value
    ; from HKCU if HKLM is set to a value other than an empty string.
    ${If} "$1" != ""
      DeleteRegValue HKCU "Software\Classes\${FILE_TYPE}"
    ${EndIf}
  ${EndIf}

>@@ -162,16 +185,24 @@ FunctionEnd
>   ${SetAppKeys}
>   ${FixClassKeys}
>   ${SetUninstallKeys}
> 
>   ; Remove files that may be left behind by the application in the
>   ; VirtualStore directory.
>   ${CleanVirtualStore}
> 
>+  ; Remove possibly badly associated file types
>+  ${FixBadFileAssociation} ".pdf"
>+  ${FixBadFileAssociation} ".oga"
>+  ${FixBadFileAssociation} ".ogg"
>+  ${FixBadFileAssociation} ".ogv"
>+  ${FixBadFileAssociation} ".pdf"
>+  ${FixBadFileAssociation} ".webm"
These should be called from the FixClassKeys macro in shared.nsh so it happens on both post update and pave over install. Also, put the FixBadFileAssociation macro above FixClassKeys so it is near the caller.

Comment 39

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #37)
> Comment on attachment 8489050 [details] [diff] [review]
> Restrict associations more to be set in less cases than currently
> 
> ># HG changeset patch
> ># Parent 74a40441372aa8e51765b74024368481eeda4334
> ># User Brian R. Bondy <[e-mail]>
> >Bug 1049521 - Only register for types when there is no default in either of HKLM or HKCU. r=rstrong
> >
> >diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
> >--- a/browser/installer/windows/nsis/shared.nsh
> >+++ b/browser/installer/windows/nsis/shared.nsh
> >@@ -426,41 +426,41 @@ FunctionEnd
> >   ${EndIf}
> > 
> >   ReadRegStr $6 SHCTX "$0\.xhtml" ""
> >   ${If} "$6" != "FirefoxHTML"
> >     WriteRegStr SHCTX "$0\.xhtml" "" "FirefoxHTML"
> >   ${EndIf}
> > 
> >   ; Only add .oga if it's not present
> >-  ${CheckIfRegistryKeyExists} "$0" ".oga" $7
> >+  ${CheckIfRegistryKeyExists} "" ".oga" $7
> >   ${If} $7 == "false"
> >     WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
> >   ${EndIf}
> There is a much simpler and more efficient way to check than what the
> CheckIfRegistryKeyExists macro uses. I'd prefer to just get rid of it after
> Seamonkey no longer uses it and use the following.
> ClearErrors
> EnumRegKey $7 HKCR ".ogv" 0
> ${If} ${Errors}
>   WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
> ${EndIf}
> 
> r=me with that change
Actually, this needs to check if there is a PersistentHandler key as well so I'd like to take a look at a new patch with that as well.

Comment 41

[Charles]

There absolutely should be an install switch that can be used to totally disable Firefox's desire to mess with my OS level file associations.

This is really starting to BUG me...

Comment 43

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #39)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #37)
> > Comment on attachment 8489050 [details] [diff] [review]
> > Restrict associations more to be set in less cases than currently
> > 
> > ># HG changeset patch
> > ># Parent 74a40441372aa8e51765b74024368481eeda4334
> > ># User Brian R. Bondy <[e-mail]>
> > >Bug 1049521 - Only register for types when there is no default in either of HKLM or HKCU. r=rstrong
> > >
> > >diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
> > >--- a/browser/installer/windows/nsis/shared.nsh
> > >+++ b/browser/installer/windows/nsis/shared.nsh
> > >@@ -426,41 +426,41 @@ FunctionEnd
> > >   ${EndIf}
> > > 
> > >   ReadRegStr $6 SHCTX "$0\.xhtml" ""
> > >   ${If} "$6" != "FirefoxHTML"
> > >     WriteRegStr SHCTX "$0\.xhtml" "" "FirefoxHTML"
> > >   ${EndIf}
> > > 
> > >   ; Only add .oga if it's not present
> > >-  ${CheckIfRegistryKeyExists} "$0" ".oga" $7
> > >+  ${CheckIfRegistryKeyExists} "" ".oga" $7
> > >   ${If} $7 == "false"
> > >     WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
> > >   ${EndIf}
> > There is a much simpler and more efficient way to check than what the
> > CheckIfRegistryKeyExists macro uses. I'd prefer to just get rid of it after
> > Seamonkey no longer uses it and use the following.
> > ClearErrors
> > EnumRegKey $7 HKCR ".ogv" 0
> > ${If} ${Errors}
> >   WriteRegStr SHCTX "$0\.oga"  "" "FirefoxHTML"
> > ${EndIf}
> > 
> > r=me with that change
> Actually, this needs to check if there is a PersistentHandler key as well so
> I'd like to take a look at a new patch with that as well.

If there is a persistent handler then it would be under the filetype for example .ogv.
So in that case there won't be an error and the WriteRegStr wouldn't be hit.
So I don't think an extra check is needed there.

I'll attach new patches though anyway because I have some other changes, and you can comment if you think that's incorrect.

Comment 44

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

That's fine. The current patch uses DeleteRegKey... would that remove the default entry under the key for that case? I'd also still like to remove the CheckIfRegistryKeyExists macro and just use the simpler method from comment #37.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Restrict associations more to be set in less cases than currently. v2

Comment 46

[Brian R. Bondy [:bbondy]]

Attachment: Fix people affected by the bug - remove already set association when there's an HKLM association. v2.

Comment 47

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #44)
> That's fine. The current patch uses DeleteRegKey... would that remove the
> default entry under the key for that case? I'd also still like to remove the
> CheckIfRegistryKeyExists macro and just use the simpler method from comment
> #37.

Yep all of those things should be handled in these new patches.

Comment 48

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

Comment on attachment 8494207 [details] [diff] [review]
Restrict associations more to be set in less cases than currently. v2

># HG changeset patch
># Parent 31a43e4f93d273c26a6d900099425181cee5cbb1
># User Brian R. Bondy <[e-mail]>
>Bug 1049521 - Only register for types when there is no default in either of HKLM or HKCU. r=rstrong
>
>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -51,16 +51,25 @@ Function RegisterStartMenuTile
>       ${If} "$AppUserModelID" != ""
>         ApplicationID::Set "$SMPROGRAMS\${BrandFullName}.lnk" "$AppUserModelID" "true"
>       ${EndIf}
>     ${EndIf}
>   ${EndIf}
> !endif
> FunctionEnd
> 
>+!macro AddAssociationIfNoneExist FILE_TYPE
>+  ClearErrors
>+  EnumRegKey $7 HKCR "${FILE_TYPE}" 0
>+  ${If} ${Errors}
>+    WriteRegStr SHCTX "$0\${FILE_TYPE}"  "" "FirefoxHTML"
Instead of relying on the caller setting $0 just hard code with SOFTWARE\Classes\

nit: I'd prefer keeping the helper macros below the main entry macros so if you could put this and the macro in the other patch further down near the caller I'd appreciate it.

r=me with that

Comment 49

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

Comment on attachment 8494207 [details] [diff] [review]
Restrict associations more to be set in less cases than currently. v2

Same here regarding putting the macro further down near the caller I'd appreciate it.

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Restrict associations more to be set in less cases than currently. v3

Carrying forward r+. Implemented review comments.

Comment 51

[Brian R. Bondy [:bbondy]]

Attachment: Fix people affected by the bug - remove already set association when there's an HKLM association. v3

This one hasn't been accepted yet, so marking for review. Moved macro to be closer to caller.
Re-tested and everything still works fine in terms of not setting default and in terms of cleaning up old people affected.

Comment 52

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

Comment on attachment 8496385 [details] [diff] [review]
Fix people affected by the bug - remove already set association when there's an HKLM association. v3

Sorry, I thought I r+'d it with comments addressed.

Comment 53

[Brian R. Bondy [:bbondy]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d574ca46990d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/403c448fcd4d

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d574ca46990d
https://hg.mozilla.org/mozilla-central/rev/403c448fcd4d

Comment 55

[zschuetz]

Will this be uplifted to ESR?

Comment 56

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

If all is well after a few days it will be requested for uplift

Comment 57

[Dimas]

Will it also be uplifted to Firefox 33?

Comment 58

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

See comment #56

Comment 59

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

Comment on attachment 8496385 [details] [diff] [review]
Fix people affected by the bug - remove already set association when there's an HKLM association. v3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: We currently take over handling of pdf files when we shouldn't which understandably upsets people.
User impact if declined: We will continue to take over handling of pdf files until this reaches esr, etc.
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): small.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 452254
[User impact if declined]: We will continue to take over handling of pdf files until this reaches esr, etc.
[Describe test coverage new/current, TBPL]: Manually tested
[Risks and why]: small because the patch has been tested and the code affected is in the external helper application that sets default handlers.
[String/UUID change made/needed]: None

Comment 61

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

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a4b04c617c

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/d126cd83b4b8

pushed to mozilla-esr31
https://hg.mozilla.org/releases/mozilla-esr31/rev/fb830a272425

Comment 62

[Ferrcad]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #61)
> [Tracking Requested - why for this release]:
> 
> [Tracking Requested - why for this release]:
> 
> Pushed to mozilla-aurora
> https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a4b04c617c
> 
> Pushed to mozilla-beta
> https://hg.mozilla.org/releases/mozilla-beta/rev/d126cd83b4b8
> 
> pushed to mozilla-esr31
> https://hg.mozilla.org/releases/mozilla-esr31/rev/fb830a272425

With all due respect can someone explain to me how to apply this fix to existing installs...  We have installed currently the ESR 31.1.1 version and we are experiencing the exact problems as described in the document.  I have tried to do some searches on how to apply these type of fixes but have not been able to successfully find a way to apply.  If someone can shoot me a URL link or directions on how to apply it would be deeply appreciated.  Also, what version of the ESR release will have this fix included?

Comment 63

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to Ferrcad from comment #62)
Link for ESR builds with this checkin:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr31/

Comment of the patch (see links with checkins in comment 61):
+; Due to a bug when associating some file handlers, only SHCTX was checked for
+; some file types such as ".pdf". SHCTX is set to HKCU or HKLM depending on
+; whether the installer has write access to HKLM. The bug would happen when
+; HCKU was checked and didn't exist since programs aren't required to set the
+; HKCU Software\Classes keys when associating handlers. The fix uses the merged
+; view in HKCR to check for existance of an existing association. This macro
+; cleans affected installations by removing the HKLM and HKCU value if it is set
+; to FirefoxHTML when there is a value for PersistentHandler or by removing the
+; HKCU value when the HKLM value has a value other than an empty string.

=> this should work when Firefox gets installed using the default NSIS installer.

Comment 64

[Ferrcad]

(In reply to XtC4UaLL [:xtc4uall] from comment #63)
> (In reply to Ferrcad from comment #62)
> Link for ESR builds with this checkin:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr31/
> 
> Comment of the patch (see links with checkins in comment 61):
> +; Due to a bug when associating some file handlers, only SHCTX was checked
> for
> +; some file types such as ".pdf". SHCTX is set to HKCU or HKLM depending on
> +; whether the installer has write access to HKLM. The bug would happen when
> +; HCKU was checked and didn't exist since programs aren't required to set
> the
> +; HKCU Software\Classes keys when associating handlers. The fix uses the
> merged
> +; view in HKCR to check for existance of an existing association. This macro
> +; cleans affected installations by removing the HKLM and HKCU value if it
> is set
> +; to FirefoxHTML when there is a value for PersistentHandler or by removing
> the
> +; HKCU value when the HKLM value has a value other than an empty string.
> 
> => this should work when Firefox gets installed using the default NSIS
> installer.

I appreciate the response but I have two problems...  I downloaded the installer from the FTP site you listed but it will install only as Nightly...  is there a way to make it install as Firefox with the changes incorporated?  Also, what option do I have for patching existing installs with the fixes listed in this bug?  Once again not real familiar with Firefox or patches/fixes.  Thanks

Comment 65

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

The fix will be provided in the next ESR release. You should be able to remove the default value of FirefoxHTML from the registry key under HKEY_CLASSES_ROOT for the file extension that you want to fix.

Comment 66

[Ferrcad]

Robert...  I have tried this but the problem we continue to see is the Firefox icon will always be associated to PDF files.  The files will open up in Reader but no matter what I have tried the icon will never be associated back to Adobe Reader.  I even tried the choose as option and selecting always and it will not change in Windows XP it stays defaulted to the Firefox icon but will open in reader when double clicking.

Comment 67

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

If there is no FirefoxHTML associated with the file extension in the registry (you might want to search the registry to verify that it isn't) then incorrect icons are usually due to the Windows icon cache not updating. You can find information on google as well as in support forums regarding this.
https://www.google.com/search?q=updating+the+windows+icon+cache

Comment 68

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've reproduced the initial issue on Win 7 x64, using Firefox 31 and ESR 31.1.1 by using the following scenario:
1. Created a new user account (alternatively can uninstall Firefox and Adobe Reader, and delete the registry entry ).
2. Installed Adobe Reader. ---> PDF's were associated with Adobe Reader (HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.pdf and HKEY_CLASSES_ROOT\.pdf became AcroExch.Document11)
3. Installed Firefox 31/31.1.1 ESR with default settings (including for being the default browser).

At this point HKEY_CLASSES_ROOT\.pdf and HKEY_CURRENT_USER\Software\Classes\.pdf became FirefoxHTML and PDFs were associated to Firefox.

I've verified that we no longer have this issue by testing the following:
a) Installation of Firefox 33 RC, 34 Aurora, 35 Nightly, and 31.2.0 ESR at step #3 no longer associates PDFs to Firefox. HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.pdf and HKEY_CLASSES_ROOT\.pdf are still AcroExch.Document11 as before, and we have no HKEY_CURRENT_USER\Software\Classes\.pdf entry.
b) Updating 31 to 33, 34, or 35, and ESR 31.1.1 to 31.2.0, while issue is present, will restore the associations and PDFs will again open with Adobe Reader. HKEY_LOCAL_MACHINE\SOFTWARE\Classes\.pdf restores to AcroExch.Document11, and HKEY_CURRENT_USER\Software\Classes\.pdf becomes empty.
c) Installation of Firefox 33 while there is NO PDF reader installed, will NOT associate PDFs to Firefox. As far as I can tell this was the desired behavior in the end.

Verification was done on Win 7 x64, for builds:
- Firefox 33 RC - BuildID=20141007073543
- Firefox 34 Aurora - BuildID=20141008004004
- Firefox 35 Nightly - BuildID=20141008030202
- Firefox 31.2.0 ESR - BuildID=20141006232702

Comment 70

[Yaron]

Not resolved in FF 33.

I removed "Firefox" from the "Open" list of ZIP files, downloaded a ZIP file, and "Firefox" is again on the list.

Comment 71

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

We don't associate as a handler of zip files. Windows does automatically add apps as handlers though I haven't seen it add us as a handler for zip files before.

Comment 72

[Yaron]

Thank you Robert.

https://bugzilla.mozilla.org/show_bug.cgi?id=1077429 has been marked as a duplicate of this bug.
The problem is still there.

See also:
http://forums.mozillazine.org/viewtopic.php?f=38&t=2870531&p=13819589#p13819589