825153 - Add support for Windows Vista WMF and prepend the system32 directory path to dll names

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 3

[Paul Adenot (:padenot)]

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

Review of attachment 696236 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/wmf/WMFDecoder.cpp
@@ -98,5 @@
> -  //       Win 7     = 6.1
> -  //       Win 8     = 6.2
> -  return versionInfo.dwMajorVersion > 6 ||
> -        (versionInfo.dwMajorVersion == 6 && versionInfo.dwMinorVersion >= 1);
> -}

Have you properly tested this feature on Windows Vista? Chris Pearce could not test this feature on a  Vista machine to make sure everything worked properly, hence this check, supposedly temporary. I don't have a Vista machine as well.

::: content/media/wmf/WMFUtils.cpp
@@ +234,5 @@
> +  uint32_t system32DirLength =
> +    GetSystemDirectoryW(system32Dir.BeginWriting(), system32Dir.Length());
> +  if (!system32DirLength) {
> +    sFailedToLoadDlls = true;
> +    return E_FAIL;

Maybe you can use |GetLastError()| so we can actually know what happens.

@@ +244,2 @@
>    for (uint32_t i = 0; i < dllLength; i++) {
> +    // Prepend the system32 directory path to prevent DLL preload attacks.

Not sure if this is necessary, since the DLL we care about are in the KnownDll list, a DLL dropped in the Firefox application directory would not be loaded. Or at least this is what [1] says. Maybe you are thinking of a different kind of attack, though, please explain.

Also, I don't really know the difference between Vista and 7 (and 8) when it comes to system directory architecture. Could you elaborate on why this is needed for Vista, but apparently not for 7 or 8, as the commit message seem to imply?

[1]: http://blogs.msdn.com/b/larryosterman/archive/2004/07/19/187752.aspx

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Paul Adenot (:padenot) [away from 29-12 to 05-01] from comment #3)
> Have you properly tested this feature on Windows Vista? Chris Pearce could
> not test this feature on a  Vista machine to make sure everything worked
> properly, hence this check, supposedly temporary. I don't have a Vista
> machine as well.
Will do.

> @@ +244,2 @@
> >    for (uint32_t i = 0; i < dllLength; i++) {
> > +    // Prepend the system32 directory path to prevent DLL preload attacks.
> 
> Not sure if this is necessary, since the DLL we care about are in the
> KnownDll list, a DLL dropped in the Firefox application directory would not
> be loaded. Or at least this is what [1] says. Maybe you are thinking of a
> different kind of attack, though, please explain.
> 
> Also, I don't really know the difference between Vista and 7 (and 8) when it
> comes to system directory architecture. Could you elaborate on why this is
> needed for Vista, but apparently not for 7 or 8, as the commit message seem
> to imply?

mfreadwrite.dll wasn't present on Vista until Platform Update Supplement is developed. So it's impossible for Vista systems without Platform Update Supplement to know about mfreadwrite.dll.

Comment 5

[Masatoshi Kimura [:emk]]

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

Hm, this patch didn't work on Vista + IE9 system.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Comment on attachment 696236 [details] [diff] [review]
> patch
> 
> Hm, this patch didn't work on Vista + IE9 system.

Do you have the "Platform update for Windows Vista" installed?

Even if you have the platform update installed, I'm not sure it'll work on Vista, I think it should though.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Chris Pearce (:cpearce, away 20 Dec until 10 Jan) from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #5)
> > Comment on attachment 696236 [details] [diff] [review]
> > patch
> > 
> > Hm, this patch didn't work on Vista + IE9 system.
> 
> Do you have the "Platform update for Windows Vista" installed?

Platform update is a prerequisite of IE9. I was not able to install "Platform update" alone if IE9 is already installed.
FWIW, the attached patch had a bug. (SetCapacity should be SetLength.) But even when I fixed the bug locally, H.264 playback still failed. (It succeeded on Windows 8 using the same binary.)

Comment 8

[Masatoshi Kimura [:emk]]

FYI, canPlayType reports that "video/mp4" is supported. So at least DLLs are loaded. But the actual playback fails.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Review of attachment 696236 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/wmf/WMFUtils.cpp
@@ +231,5 @@
> +  // Prepare the system32 directory path.
> +  nsAutoString system32Dir;
> +  system32Dir.SetCapacity(MAX_PATH);
> +  uint32_t system32DirLength =
> +    GetSystemDirectoryW(system32Dir.BeginWriting(), system32Dir.Length());

Passing system32Dir.Length() here won't work, as it returns 0, it's the capacity that you want to pass. So you need to pass MAX_PATH instead of the result of Length(), otherwise GetSystemDirectoryW thinks it's input buffer is 0 length.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

I had a go at getting the WMF backend to work on Vista, here's the results of my attempts.

I'm building on Windows 8 with Visual Studio 2012. This uses Windows SDK version 8.0.

I installed Vista in a VM and fully patched it with Windows Update. It definitely has the Platform Update for Windows Vista installed. IE9 can play H.264 HTML5 video.

I built Firefox with the emk's patch. When running on Vista the call to MFStartup() fails, it returns MF_E_BAD_STARTUP_VERSION.

Fair enough, in WMF.h I've defined:
#define WINVER _WIN32_WINNT_WIN7
http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMF.h#20

If I change WINVER to _WIN32_WINNT_VISTA, I get compile errors:

error C2027: use of undefined type 'IMFSourceReader' [...]
C:\Program Files (x86)\Windows Kits\8.0\include\um\mfreadwrite.h(52) : see declaration of 'IMFSourceReader'

It turns out the definition of IMFSourceReader is inside an #if (WINVER >= _WIN32_WINNT_WIN7) block.

But according to MSDN IMFSourceReader is supposed to be available on Windows Vista. :(

(Note the mfreadwrite.h header in the Windows 7.0 SDK has the same _WIN32_WINNT_WIN7 guard).

If I change the header (mfreadwrite.h) so that #if is (WINVER >= _WIN32_WINNT_VISTA) I can compile successfully.

In this build still the MFStartup() call still fails on Vista, it returns MF_E_BAD_STARTUP_VERSION.

According to this page:
http://msdn.microsoft.com/en-us/library/windows/desktop/ee663600%28v=vs.85%29.aspx
I should be statically linking against mf_vista.lib and mfplat_vista.lib to access. I tried that, and MFStartup still returns MF_E_BAD_STARTUP_VERSION on Vista.

My googling hasn't been productive, I've found several posts by people with the same problem, but no answers.

I'm going to try building with earlier SDK versions, but I don't have much hope for that.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Chris Pearce (:cpearce) from comment #9)
> Passing system32Dir.Length() here won't work, as it returns 0, it's the
> capacity that you want to pass. So you need to pass MAX_PATH instead of the
> result of Length(), otherwise GetSystemDirectoryW thinks it's input buffer
> is 0 length.

Yeah, it was I said "Fixed it locally but didn't still work" bug in comment #7.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ah of course, sorry I missed that on re-reading.

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to Chris Pearce (:cpearce) from comment #10)
> I had a go at getting the WMF backend to work on Vista, here's the results
> of my attempts.
> 
> I'm building on Windows 8 with Visual Studio 2012. This uses Windows SDK
> version 8.0.
> 
> I installed Vista in a VM and fully patched it with Windows Update. It
> definitely has the Platform Update for Windows Vista installed. IE9 can play
> H.264 HTML5 video.
> 
> I built Firefox with the emk's patch. When running on Vista the call to
> MFStartup() fails, it returns MF_E_BAD_STARTUP_VERSION.
> 
> Fair enough, in WMF.h I've defined:
> #define WINVER _WIN32_WINNT_WIN7
> http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMF.h#20
> 
> If I change WINVER to _WIN32_WINNT_VISTA, I get compile errors:
> 
> error C2027: use of undefined type 'IMFSourceReader' [...]
> C:\Program Files (x86)\Windows Kits\8.0\include\um\mfreadwrite.h(52) : see
> declaration of 'IMFSourceReader'
> 
> It turns out the definition of IMFSourceReader is inside an #if (WINVER >=
> _WIN32_WINNT_WIN7) block.
> 
> But according to MSDN IMFSourceReader is supposed to be available on Windows
> Vista. :(
> 
> (Note the mfreadwrite.h header in the Windows 7.0 SDK has the same
> _WIN32_WINNT_WIN7 guard).
> 
> If I change the header (mfreadwrite.h) so that #if is (WINVER >=
> _WIN32_WINNT_VISTA) I can compile successfully.
> 
> In this build still the MFStartup() call still fails on Vista, it returns
> MF_E_BAD_STARTUP_VERSION.
> 
> According to this page:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ee663600%28v=vs.
> 85%29.aspx
> I should be statically linking against mf_vista.lib and mfplat_vista.lib to
> access. I tried that, and MFStartup still returns MF_E_BAD_STARTUP_VERSION
> on Vista.
> 
> My googling hasn't been productive, I've found several posts by people with
> the same problem, but no answers.
> 
> I'm going to try building with earlier SDK versions, but I don't have much
> hope for that.

This confusion is really due to a header #fail on Microsoft's part. Just because you're using a certain SDK doesn't mean you are only targeting that associate Windows version.  They should have made 2 defines for vista and win7 plus and allowed you to set it, or even better just detected themselves within the function :(

Anyway fix being attached now which works in both Vista and Win7+.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - WMF Vista support + load libraries from system32 directly

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Is vista check

I'd like to eventually move the widget helper OS checks here and cleanup the code throughout the tree, but for now just adding a small helper similar to the current IsVistaOrLater.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #15)
> Created attachment 729270 [details] [diff] [review]
> Patch v1 - Is vista check
> 
> I'd like to eventually move the widget helper OS checks here and cleanup the
> code throughout the tree, but for now just adding a small helper similar to
> the current IsVistaOrLater.

I actually prefer the WinUtils version checks over functions like this. In fact, you can use those now and not add anything to this file since WinUtils.h is now exported.

Comment 17

[Brian R. Bondy [:bbondy]]

ok nice, I'll update to that and in that case I won't need a review from you :D

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - WMF Vista support + load libraries from system32 directly

Updated as per Jim's suggestion for OS detection.

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 729339 [details] [diff] [review]
Patch v2 - WMF Vista support + load libraries from system32 directly

Review of attachment 729339 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/wmf/WMFUtils.cpp
@@ +273,5 @@
>  {
>    RefPtr<IMFPluginControl> pluginControl;
>    HRESULT hr = wmf::MFGetPluginControl(byRef(pluginControl));
> +  if (SUCCEEDED(hr) && pluginControl) {
> +    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);

This |NS_ENSURE_TRUE| is now useless, since we go in this if only if |hr| is 0.

Comment 20

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad472cef6e89

Comment 21

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ad472cef6e89

Comment 22

[Jesper Kristensen]

I added Vista on https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats