Closed Bug 523410 Opened 10 years ago Closed 9 years ago

Disable LSPs in WinSock that don't have categories for Firefox on Windows Vista and above

Categories

(Core :: Networking, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: vlad, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [crashkill][crashkill-thirdparty])

Attachments

(1 file, 5 obsolete files)

A number of our crashes are due to Winsock Layered Service Providers, which are essentially DLLs that get to inject themselves into the winsock stack at arbitrary points.  They are known fragile, and are frequently used as malware/spyware injection points.  A more robust alternative, the Windows Filtering Platform is available for Vista and newer: http://www.microsoft.com/whdc/device/network/wfp.mspx 

According to http://blogs.msdn.com/wndp/archive/2006/02/09/529031.aspx we should be able to call WSCSetApplicationCategory to limit the categories of LSPs that are loaded.  I believe if we specify only the SYSTEM category, we avoid everything else.  However, there are access control issues with this API call, as seen in http://msdn.microsoft.com/en-us/library/ms742253%28VS.85%29.aspx -- so we may need to make this call at install/update time (when we're running with elevated privileges).  The setting does persist.
blocking2.0: --- → ?
Ted just found http://www.ureader.com/msg/14772261.aspx which seems to imply that IE doesn't load LSPs when running in low rights mode.  We should experiment to see what happens with Firefox when running as admin vs as a regular user, and as a regular user with UAC enabled or disabled.
isn't AV software using LSPs to filter the incoming http stream ?
AV software would (should?) have system-level privileges, though, right?
Also: don't really care.  AV software is more often than not one of the things that crashes us, and if IE isn't loading it, then we don't need to either.  We need to stand by Firefox being secure, which means that you shouldn't need any other software to be secure.
This definitely looks like it's worth experimenting with.
Yeah, i'm all in favor of doing this.
Blocks: 467167
I don't really by the argument that we should be secure on our own without help from AV software. Unless we're planning on becoming an AV vendor, I don't see us scanning downloaded files for viruses/malware for example.
Vlad: Is there anything we can do here in time for 3.6? I would still be concerned if this meant locking out AV software, but comment 3 seems to indicate that we could do this without blocking AV software. Or maybe there is a way we could block some categories without blocking AV software.

If you think this could be done in the 3.6 timeframe, would you have time to do it?

cc'ing Rob Strong and Jim Mathies to see if they have ideas/cycles.
Doubt I will have any time... taking tomorrow off and I'm trying to get a handle on bug 501429.
Some AV LSPs are crashing us if I read https://bugzilla.mozilla.org/show_bug.cgi?id=467167#c49

- sockspy.dll - known for crashing browsers, BitDefender, anti-virus
- SpSubLSP.dll - anti-spam product


File Downloads are safe because the AV software should catch the file on the hdd. Only the detecting of code that tries to exploit security holes in Gecko and in plugins would be affected if AV LSPs are blocked.
Whiteboard: [crashkill][crashkill-thirdparty]
bug 514612 is another example of an LSP that's causing crashes, and one that's pretty high on our topcrash list.
Yeah, I'm leaning towards that we should block LSPs outright. But we need to do it early in a release cycle (such as now) in order to get feedback from users and LSP vendors.
Marking this a blocker, but we don't have anyone who can work on this at the moment. We'll be looking for someone to do this work, but if we're unable to find someone to work on this in time for the release, we may need to push this to the next release.
blocking2.0: ? → beta1
This is pretty straightforward to actually do, the hard part is getting the testing done as sicking says in Comment 12.

I could put together a simple patch to the updater this weekend to call WCSSetApplicationCategory on each update (I don't know enough about NSIS to be able to do the same to the installer easily).  Would we want to block all LSPs?  That API gives us the ability to change our category based on the command line so we could do:

firefox.exe = load only LSP_SYSTEM LSPs
firefox.exe -safe-mode = load no LSPs

Personally I think we should just block them all all the time though.
Kyle, it would be awesome if you could write up a patch! I'd imagine there's people here that can help you with the NSIS part even.

I don't have an opinion on system-only vs. blocking all, because I don't really know what's required for an LSP to be system-only.

Do you or anyone else here know what it means for an LSP to be a system LSP? I.e. do you need to get signed by microsoft or some such? Do we know if various legit, but low quality LSPs (like bug 527540) are system LSPs?
(In reply to comment #15)
> I don't have an opinion on system-only vs. blocking all, because I don't really
> know what's required for an LSP to be system-only.
> Do you or anyone else here know what it means for an LSP to be a system LSP?

From my understanding, the API relies on the LSP to vouch for itself.  I do not believe there is any requirement other than calling the corresponding LSP API call (WCSSetProviderCategory I believe) and marking yourself as LSP_SYSTEM.
If that's the case then I say lets block them all. If that's too strict we can always reevaluate later.
Please do not add a special --safe-mode case because it's makes it harder to find the cause of issues.
(In reply to comment #18)
> Please do not add a special --safe-mode case because it's makes it harder to
> find the cause of issues.

(In reply to comment #17)
> If that's the case then I say lets block them all. If that's too strict we can
> always reevaluate later.

Noted.  I'll give this a shot this weekend.
Thank Kyle! Mad props if you are able to get this working.
I have a patch for this that's going through try now.  I have a local LSP that's currently loaded into Firefox and other user mode winsock-consuming processes but isn't loaded into lsass.exe which I'm going to use to test this.

I'm not sure if calling WSCSetApplicationCategory with 0 for the permitted LSP category means "don't load any LSPs at all" or "don't set an application category" so I've fired off a build with 0 and LSP_SYSTEM; it's possible that restricting the loaded LSPs to LSP_SYSTEM-marked LSPs will be the best we can do. The fact that lsass.exe marks itself as accepting LSP_SYSTEM LSPs leads me to believe that it may not be possible to reject all LSPs.
Assignee: nobody → me
Status: NEW → ASSIGNED
So after a lot of experimenting this is what I've determined:

Winsock stores this information in HKLM/SYSTEM/ControlSet001/services/WinSock2/Parameters/AppID_Catalog/<what appears to be a hash of the application's name>/

The patch I'm about to attach calls WSCSetApplicationCategory to set the permitted types of LSPs to load into Firefox to LSP_SYSTEM.  After experimenting I determined that calling the API with no LSP types specified means "open the gates" (it clears out the registry entries) so this is the best we're going to get.

The API creates the registry entries in that key mirroring those for lsass.exe (which is also set to LSP_SYSTEM).  It still appears that the LSPs are loaded into our process though.  I even tried rebooting to see if Winsock picks up these registry entries on startup but the LSP DLLs were still loaded.

Among the possible things I can think of for this:
1) I'm running on x64 and Firefox is a 32 bit app.  Winsock has two completely different stacks for 32 and 64 bit applications so maybe there's some weird interaction here.
2) All the LSPs on my system are marked LSP_SYSTEM (though this doesn't explain why they don't get loaded into lsass.exe)
3) The DLLs are still loaded into memory but not hooked up to the Winsock stack.  Again doesn't explain why they don't get loaded into lsass.exe

The two options forward that I see are either to find some installations affected by bad LSPs and install a build with the forthcoming patch there, or to drop it into trunk and see what happens.
Attached patch To LSPs with love (obsolete) — Splinter Review
The name is shamelessly stolen from Jonas.

I suppose the best person to review the actual code changes is Robert Strong since it will live in the installer.  I'd like comments from Jonas et. al. about the above and how we want to proceed too.
Attachment #425685 - Flags: review?(robert.bugzilla)
Attached patch To LSPs with love 1.1 (obsolete) — Splinter Review
Really need to remember to qrefresh first.
Attachment #425685 - Attachment is obsolete: true
Attachment #425686 - Flags: review?(robert.bugzilla)
Attachment #425685 - Flags: review?(robert.bugzilla)
Comment on attachment 425686 [details] [diff] [review]
To LSPs with love 1.1

A couple of quick review comments.

I highly suspect that it is actually stored under CurrentControlSet and not ControlSet001... iirc ControlSet001, ControlSet002, etc. are the backup of CurrentControlSet which are used to revert to a previous configuration. Can you verify this?

I'm leaning to make the calls to KillLSPs per app so it would be opt-in. It would then be called from browser/installer/windows/nsis/ installer.nsi (towards the end of the install) and shared.nsh (in PostUpdate).

I am quite sure this can only be called when the user has write access to HKLM. It is somewhat easy to find code that already does this in the two files. As this currently stands 

On failure during install it should log that it failed to the install.log
(In reply to comment #26)
> (From update of attachment 425686 [details] [diff] [review])
> A couple of quick review comments.
> 
> I highly suspect that it is actually stored under CurrentControlSet and not
> ControlSet001... iirc ControlSet001, ControlSet002, etc. are the backup of
> CurrentControlSet which are used to revert to a previous configuration. Can you
> verify this?
> 
Yes, you are correct.

> I'm leaning to make the calls to KillLSPs per app so it would be opt-in. It
> would then be called from browser/installer/windows/nsis/ installer.nsi
> (towards the end of the install) and shared.nsh (in PostUpdate).
> 
OK.

> I am quite sure this can only be called when the user has write access to HKLM.
> It is somewhat easy to find code that already does this in the two files. As
> this currently stands 
> 
> On failure during install it should log that it failed to the install.log

Yes, when the user doesn't have HKLM write access (or something else goes wrong) the function returns -1 and sets the outparameter to an error code.  If all we care about is the code we can log that, otherwise we can call FormatMessage to get a real string.  Any preference?
The code should be sufficient
Is there a decent way to remove this on uninstall? If not I think we should hack removing the added registry settings into the uninstaller.
Kyle, what LSPs were you trying with to see if they get loaded?

I take it this is happening even in the latest patch?
(In reply to comment #29)
> Is there a decent way to remove this on uninstall? If not I think we should
> hack removing the added registry settings into the uninstaller.

Yes, we should just call WSCSetApplicationCategory against with 0 as the argument and the system will clean it up for us.

(In reply to comment #30)
> Kyle, what LSPs were you trying with to see if they get loaded?
> 
> I take it this is happening even in the latest patch?

My system has the following LSPs installed: (According to the LSP-Fix tool)

NLAapi.dll "Network Location Awareness 2" (From Microsoft, ships with the OS)
mswsock.dll "Microsoft Windows Sockets 2 Service Provider" (Appears to be a compatability shim)
winrnr.dll "LDAP RNR Provider DLL" (Also from MS, ships with the OS)
napinsp.dll "Email naming shim provider" (Again from MS, ships with the OS)
pnrpnsp.dll "PNRP Namespace Provider" (...)
wshbth.dll "Windows Sockets Helper DLL" (...)
WLIDNSP.dll "Windows Live ID Namespace Provider" (From MS, but part of a Windows Update)

My definition of ships with the OS is that the DLL has the same version number as all the other Windows 7 DLLs (6.1.7600.16385)

All of these DLLs are loaded into Firefox, only mswsock.dll is loaded into lsass.exe, even though they have identical registry entries in the keys I mentioned earlier.
Attachment #425686 - Attachment is obsolete: true
Attachment #425686 - Flags: review?(robert.bugzilla)
One thing that occurred to me. Does Chrome stop LSPs from getting loaded? If so we can simply check how they are doing it, or probably even ask them.
Chrome might, but Chromium does not.
I was thinking about "theory 1" in comment 22. Is lsass.exe a 32bit app too? If so that doesn't really explain what you are seeing. If not, maybe we could get someone to test your patch on a 32bit system.

Is there an easy way to test your patch?
(In reply to comment #34)
> I was thinking about "theory 1" in comment 22. Is lsass.exe a 32bit app too? If
> so that doesn't really explain what you are seeing. If not, maybe we could get
> someone to test your patch on a 32bit system.
> 
> Is there an easy way to test your patch?

No, it's a 64 bit app.  To test it all you would need to do is install the tryserver build I posted a while back (you have to install it because this code lives in the installer) and then use something like Process Explorer to see what DLLs are loaded into Firefox.  You can use LSP Fix or a similar program to get a list of LSPs and see what is loaded.
I can probably test that myself this weekend actually.
I never did figure out why the API call didn't work, and I'm no longer working on this, so moving to nobody@mozilla.org.
Assignee: me → nobody
Status: ASSIGNED → NEW
blocking2.0: beta1+ → beta2+
This isn't going to happen for Firefox 4, unless someone steps up to do this work, or even explains how this could be fixed.
blocking2.0: beta2+ → -
Kyle, I just tried out the minimal amount of code from the last patch (ran it as admin of course) and it did add the entry for this in the registry under HKLM\SYSTEM\CurrentControlSet\services\WinSock2\Parameters\AppId_Catalog\<generated id>\

with AppFullPath set correctly and PermittedLspCategories = 0x80000000

Same settings as used by lsass.exe as a matter of fact.

I wonder if the method to test if it took affect is the problem?

I also noticed that when it runs without admin privs the installer crashes... might be better to use a NSIS plugin to accomplish this to avoid the crash :(
For the hell of it I installed an old SDK LSP that prevents connecting to anything and verified that Minefield, Namoroka, and Shiretoko all could not connect. I then used the patch for each of these install locations and I was then able to connect with each of them.

I also verified that the registry entries are removed when specifying 0x0.

What are your thoughts on this? Perhaps the LSPs you were testing with were in the system category?
Completely possible that my testing methodology was bogus. (See comment 22 theory 2).  My method was essentially to compare the LSPs in lsass and firefox and ensure that they are the same, but it's possible that lsass does other weird stuff too.

(In reply to comment #40)
> For the hell of it I installed an old SDK LSP that prevents connecting to
> anything and verified that Minefield, Namoroka, and Shiretoko all could not
> connect. I then used the patch for each of these install locations and I was
> then able to connect with each of them.
> 
> I also verified that the registry entries are removed when specifying 0x0.
> 
> What are your thoughts on this? Perhaps the LSPs you were testing with were in
> the system category?

This sounds good.  Let's clean this up and get it in the tree then ;-)
Rob, should I pick this up and finish off the review comments? (and make my first NSIS plugin?)
If you have the time that would be great but I'm not convinced this actually has to be a plugin. Instead, you could just test if you can write to the following before making the call.
HKLM\SYSTEM\CurrentControlSet\services\WinSock2\Parameters\AppId_Catalog\
I also verified it works with
http://www.komodia.com/index.php?page=sniffer.htm

without the LSP restriction it is able to sniff all http(s) and with the LSP restriction it is unable to.
(In reply to comment #44)
> I also verified it works with
> http://www.komodia.com/index.php?page=sniffer.htm
> 
> without the LSP restriction it is able to sniff all http(s) and with the LSP
> restriction it is unable to.
Just tried it again and it was able to sniff... not sure what is going on with it but I did try the first test as described in comment #40 multiple times.
Attached patch To LSPs with love 2.0 (obsolete) — Splinter Review
Haven't tested the update path, but the other paths work fine.
Assignee: nobody → me
Status: NEW → ASSIGNED
Attachment #458581 - Flags: review?(robert.bugzilla)
Couple of notes:
It appears that IE doesn't bypass LSP's when retrieving the favicon.
I suspect IE is going through some of the LSP categories... I'll register my test LSP as each of the possible categories to try to find out which ones.

After getting a better handle on things tonight (everyone, ignore any conversation I had with you today if you don't already ;) I *think* we can do this and get some benefit but I would like to duplicate IE's categorization if possible. Regretfully, it isn't obvious if IE is registered to only receive some categories so it will have to be checked by testing it with each one.
Managed to get a few more details.

The anomalies I saw with IE only going through the LSP for the favicon was due to having "Protected Mode On" (suspect that the IE process for retrieving the favicon uses a process that isn't low integrity) and trying to log to a file that the Protected Mode process didn't have write access to. After I changed the log location to a low integrity part of the file system everything from IE was logged. Running as admin logged in both locations.

IE does in fact load all LSP's (it doesn't specify specific LSP categories) though the LSP itself might not be coded to handle protected mode.

One thing we could do is only load LSP's on Vista and above that have a category which should remove all of the old LSP's that are problematic though it would also remove valid ones that aren't problematic that haven't been updated to use categories.

List of all categories:
http://msdn.microsoft.com/en-us/library/bb513664%28VS.85%29.aspx#lsp_categories

Thoughts?
btw: if anyone has a link to download a misbehaving LSP that affects Firefox and doesn't require specific hardware (not positive but it looks like the nVidia LSP is hardware specific) I'd appreciate if you could provide a link so I can verify the fix.
Looking through the crash reports for _PR_MD_SEND I found the following Firefox 4.0b1 that has GoogleDesktopNetwork3.dll in the stack.
http://crash-stats.mozilla.com/report/index/6b98d01c-94d0-4023-8e43-427432100720

I thought we prevented that from loading.
(In reply to comment #50)
> Looking through the crash reports for _PR_MD_SEND I found the following Firefox
> 4.0b1 that has GoogleDesktopNetwork3.dll in the stack.
> http://crash-stats.mozilla.com/report/index/6b98d01c-94d0-4023-8e43-427432100720
> 
> I thought we prevented that from loading.

Only unversioned instances.

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsDllBlocklist.h#90

Perhaps that needs to be revisited in another bug?
We are, but only dlls that don't have any marked version. See

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsDllBlocklist.h#92

Maybe google released a new version.
I think we should move forward with this by evaluating it on trunk and go with all LSP_*'s except for LSP_SYSTEM. I wouldn't be surprised if this breaks google desktop.

I'll review the patch tonight or tomorrow. As it stands the value has to be cleared first because it is cumulative believe it or not. Also, I think the value to set should be passed in by the caller.
Google desktop may use an LSP, don't know, but they also use another API that hooks right into the HTML parser, which changed when the HTML5 parser landed (or never got implemented), and even before then we stopped loading the google desktop dll into the Firefox process (it wasn't a registered component etc), so google desktop hasn't worked for a long time, and there has been very little sign that they care to update it either, so based on that this change isn't going to make things much worse for google desktop.
jst: see comment #50... it appears that they have a version out now that is being loaded.
Comment on attachment 458581 [details] [diff] [review]
To LSPs with love 2.0

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -126,16 +126,18 @@ VIAddVersionKey "OriginalFilename" "setu
> !insertmacro InstallEndCleanupCommon
> !insertmacro InstallOnInitCommon
> !insertmacro InstallStartCleanupCommon
> !insertmacro LeaveDirectoryCommon
> !insertmacro LeaveOptionsCommon
> !insertmacro OnEndCommon
> !insertmacro PreDirectoryCommon
> 
nit: remove the empty line.
>+!insertmacro KillLSPs
Let's go with SetAppLSPCategories


>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
>@@ -29,16 +29,18 @@
> # use your version of this file under the terms of the MPL, indicate your
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
>+!insertmacro KillLSPs
Don't add this in shared.nsh and instead add it to uninstaller.nsi with the rest. There is a reason for this if you want to hear it. ;)

>diff --git a/browser/installer/windows/nsis/uninstaller.nsi b/browser/installer/windows/nsis/uninstaller.nsi
>--- a/browser/installer/windows/nsis/uninstaller.nsi
>+++ b/browser/installer/windows/nsis/uninstaller.nsi
>@@ -118,16 +118,17 @@ VIAddVersionKey "OriginalFilename" "help
> !insertmacro un.ParseUninstallLog
> !insertmacro un.RegCleanAppHandler
> !insertmacro un.RegCleanFileHandler
> !insertmacro un.RegCleanMain
> !insertmacro un.RegCleanUninstall
> !insertmacro un.RegCleanProtocolHandler
> !insertmacro un.RemoveQuotesFromPath
> !insertmacro un.SetBrandNameVars
>+!insertmacro un.KillLSPs
When you change this to !insertmacro un.SetAppLSPCategories and add !insertmacro SetAppLSPCategories please add them sorted with the rest.
In that case I suspect they have more than one dll, registered in different ways.
The ws2_32.dll infers that it made it into our process via its LSP (it does have one iirc) and since we block unversioned GoogleDesktopNetwork3.dll I suspect this one is versioned.
Attachment #458581 - Attachment is obsolete: true
Attachment #458581 - Flags: review?(robert.bugzilla)
Talked with Kyle about this and he agreed to let me take this from him since I want to add some complexity to the patch to protect against crashing and such.
Assignee: me → robert.bugzilla
OS: Windows NT → Windows Vista
Wikipedia says that Vista's parental controls are implemented as an LSP. Is this a problem?
I planned on verifying that it isn't along with a couple of other LSPs and as long as they use a category for the LSP it won't be a problem.
Apologies if I'm way out of my depth here but I was just read this bug and was wondering if it is wise to disable functionality that people may be relying on? Looking at the categories on http://msdn.microsoft.com/en-us/library/bb513664%28VS.85%29.aspx#lsp_categories I was wondering if this covers things such as VPN clients as well as AV software? Is it possible to just disable just LSPs that are known to be causing crashes?
I'm concerned about this as well so we are going to try this out on the trunk nightly builds and if all goes well on the next beta. There are a lot of LSPs out there that are badly designed / not updated and the hope is that the well designed ones will have been updated to use categories which was implemented in Windows Vista.

The difficulty with just disabling LSPs that cause problems is that they have to be done one by one as they are found.

So, if the benefit doesn't outweigh the value of the LSPs that don't implement categories this will likely be backed out but to find this out we are going to try it out.
if we run lsp's in a specific process we could recognize when the process crashes and then slowly tune lsp's out when we hit problems for a given user.

our crash reporting and other library blocking stuff can support stuff like this.

but for now, we definitely want to get this in on trunk and see how things go. adaptive feedback takes more time+effort and will have a smaller win for hopefully a rare case.
Summary: Disable LSPs in WinSock for Firefox → Disable LSPs in WinSock that don't have categories for Firefox on Windows Vista and above
blocking2.0: - → beta3+
Robert, do you think this can make the Monday deadline? If not, I'll mark this beta4+, but given the nature of this change I'd like it in a beta sooner rather than later.
I'll have a patch very soon as in likely tomorrow and the only concern is getting a review.
Rob, what news here?
Other blockers ate my weekend... I'm working on it and jst said if it isn't ready for B3 he would move it to B4.
blocking2.0: beta3+ → beta4+
Attached patch patch rev1 (obsolete) — Splinter Review
Kyle, just found out that jimm is on vacation and I'd like to get this in sooner rather than later so if you could review this it would be appreciated.
Attachment #464660 - Flags: review?(me)
Comment on attachment 464660 [details] [diff] [review]
patch rev1

Forgot to add a header for the install log and a macro header... will resubmit after that is done
Attachment #464660 - Attachment is obsolete: true
Attachment #464660 - Flags: review?(me)
Attached patch patch rev2 (obsolete) — Splinter Review
Kyle, I also cleaned up some the logging calls that weren't using the logging macros.
Attachment #464707 - Flags: review?(me)
Also verified this work with NSIS 2.33 and NSIS 2.46 and it doesn't crash if the user doesn't have privileges
Comment on attachment 464707 [details] [diff] [review]
patch rev2

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -393,16 +393,21 @@ Section "-Application" APP_IDX
>   ; an install into a different location.
>   StrCpy $0 "Software\Microsoft\Windows\CurrentVersion\App Paths\${FileMainEXE}"
>   ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0
>   ${WriteRegStr2} $TmpVal "$0" "Path" "$INSTDIR" 0
> 
>   StrCpy $0 "Software\Microsoft\MediaPlayer\ShimInclusionList\$R9"
>   ${CreateRegKey} "$TmpVal" "$0" 0
> 
>+  ${If} $TmpVal == "HKLM"
>+    ; Set the permitted LSP Categories for WinVista and above
>+    ${SetAppLSPCategories} ${LSP_CATEGORIES}
>+  ${EndIf}
>+
>   ; Create shortcuts
>   ${LogHeader} "Adding Shortcuts"
> 
>   !insertmacro MUI_STARTMENU_WRITE_BEGIN Application
> 
>   ; Always add the relative path to the application's Start Menu directory and
>   ; the application's shortcuts to the shortcuts log ini file. The
>   ; DeleteShortcuts macro will do the right thing on uninstall if they don't

Is there a reason this can't be in the HKLM block above?

Other than that, looks great.  We should test this with the Vista/7 parental controls before we land.
Attachment #464707 - Flags: review?(me) → review+
(In reply to comment #73)
>...
> Is there a reason this can't be in the HKLM block above?
This way it is added to the log after the registry entries section so it has its own section

> Other than that, looks great.  We should test this with the Vista/7 parental
> controls before we land.
Yoou had to remind me about that didn't you. ;)
(In reply to comment #74)
> (In reply to comment #73)
> >...
> > Is there a reason this can't be in the HKLM block above?
> This way it is added to the log after the registry entries section so it has
> its own section

OK, SGTM.

> > Other than that, looks great.  We should test this with the Vista/7 parental
> > controls before we land.
> Yoou had to remind me about that didn't you. ;)

Yes, yes I did. ;-)
Just verified parental controls work on Win7 so I am going to go with it
(In reply to comment #76)
> Just verified parental controls work on Win7 so I am going to go with it

Awesome.
Added push info in case this can ride along with someone elses push. Carrying forward r+
Attachment #464707 - Attachment is obsolete: true
Attachment #465536 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/173fb265a91c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
I've asked Juan to keep an eye out for reports of unusual behavior with 3rd party app integration with Firefox.
cc'ing chofmann

A couple of the dll's I would hope to not see on Vista and above with beta 4 are
BkavHook.dll
vksaver.dll
This virtually eliminated our _PR_MD_SEND crashes on Vista and 7 :-)
Status: RESOLVED → VERIFIED
Holy <insert expletive here>, For beta 7 and 8 there are only 5 crashes on WinVista / Win7 out of 2366 crashes containing _PR_MD_SEND in the last week.
So, looking at 3.6.x numbers it appears that there are significantly fewer crashes containing _PR_MD_SEND on WinVista / Win7. I think the data needs to be looked at a bit more.
note: crash-stats for 3.6.12 for _PR_MD_SEND have 6532 crashes for the top most result of just _PR_MD_SEND. Out of that 6532 there are only 18 crashes for Windows NT 6.0 and Windows NT 6.1.
Blocks: 630481
Blocks: 621320
Blocks: 636088
Reopening since the functionality has for all intents and purposes been backed out in bug 636088
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee: robert.bugzilla → nobody
I don't think there's much value in leaving this open.  We can revisit this if in the future our LSP crashes on Vista+ spike or if the compatibility concerns diminish.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → INCOMPLETE
Duplicate of this bug: 1234931
You need to log in before you can comment on or make changes to this bug.