1325980 - Intel driver 15.45 produces GFX assert error

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oriol Brufau [:Oriol]]

Some months ago I could build and run Firefox without problems.

Now I tried again and it crashes due to this assert in GfxInfo.cpp
https://dxr.mozilla.org/mozilla-central/rev/3119a9a0b5dee60ac77b7596ae5dbe0658f598ad/widget/windows/GfxInfo.cpp#601
> } else if (dllNumericVersion == 0 && dllNumericVersion2 == 0) {
>   // Leave it as an asserting error for now, to see if we can find
>   // a system that exhibits this kind of a problem internally.
>   gfxCriticalErrorOnce() << "Potential driver version mismatch ignored due to missing DLLs "
>                          << NS_ConvertUTF16toUTF8(dllVersion).get() << " and "
>                          << NS_ConvertUTF16toUTF8(dllVersion2).get();
> }

So such a system has been found.

I tried to build an older revision of Firefox which some months ago I could run without problems, but now I get the same problem.

So the problem has not been caused by some change in Firefox.

I suspect it could be that I updated my graphic drivers or something like that.

This is my graphics info
> Features
>  - Compositing:                Direct3D 11
>  - Asynchronous Pan/Zoom:      none
>  - WebGL Renderer:             Google Inc. -- ANGLE (Intel(R) HD Graphics 530 Direct3D11 vs_5_0 ps_5_0)
>  - WebGL2 Renderer:            Google Inc. -- ANGLE (Intel(R) HD Graphics 530 Direct3D11 vs_5_0 ps_5_0)
>  - Hardware H264 Decoding:     Yes; Using D3D11 API
>  - Audio Backend:              wasapi
>  - Direct2D:                   true
>  - DirectWrite:                true (10.0.14393.351)
> GPU #1
>  - Active:                     Yes
>  - Description:                Intel(R) HD Graphics 530
>  - Vendor ID:                  0x8086
>  - Device ID:                  0x1912
>  - Driver Version:             21.20.16.4542
>  - Driver Date:                10-24-2016
>  - Drivers:                    igdumdim64 igd10iumd64 igd10iumd64 igd12umd64 igdumdim32 igd10iumd32 igd10iumd32 igd12umd32
>  - Subsys ID:                  00000000
>  - RAM:                        Unknown
> Diagnostics
>  - AzureCanvasAccelerated:     0
>  - AzureCanvasBackend:         direct2d 1.1
>  - AzureContentBackend:        direct2d 1.1
>  - AzureFallbackCanvasBackend: skia
> Decision Log
>  - D3D9_COMPOSITING:           disabled by default: Disabled by default
>  - GPU_PROCESS:                unavailable by runtime: Multi-process mode is not enabled

The debugger shows these variables:
> - dllNumericVersion     unsigned __int64    0
> - dllNumericVersion2    unsigned __int64    0
> - dllVersion            nsString            {...}
>   - nsAString_internal  nsAString_internal  {...}
>     - mData             char16_t *          0x64e0dbf0 u"0.0.0.0"
>       -                 char16_t            48 u'0'
>     - mLength           unsigned int        7
>     - mFlags            unsigned int        33
> - dllVersion2           nsString            {...}
>   - nsAString_internal  nsAString_internal  {...}
>     - mData             char16_t *          0x64e0dbf0 u"0.0.0.0"
>       -                 char16_t            48 u'0'
>     - mLength           unsigned int        7
>     - mFlags            unsigned int        33

Tell me if you need more data

Comment 1

[Oriol Brufau [:Oriol]]

This is definitely due to the new 15.45 driver.

I downgraded from 15.45.10.4542 to 15.40.28.4501 and now there is no such problem.

Comment 2

[Oriol Brufau [:Oriol]]

The problem is that the new version removes
C:\Windows\SysWOW64\igd10iumd32.dll

The 64-bit one is not removed
C:\Windows\System32\igd10iumd64.dll

So probably this problem only happens on 64-bit systems with the 64-bit driver but a 32-bit Firefox.

Comment 3

[Oriol Brufau [:Oriol]]

Driver 15.45.14.4590 (21.20.16.4590) for 64-bit removes
C:\Windows\System32\igd10iumd64.dll

So now both 32-bit and 64-bit versions of Firefox have this failed GFX assert.

Comment 4

[Markus Stange [:mstange]]

Looks like Milan added the comment and the error.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

What are the values of dllFileName and dllFileName2 when the assert hits?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Maybe the DLLs don't match the driver version numbers anymore, just the build ids (last four digits), so we would need to change the test to only check that.

Comment 7

[Oriol Brufau [:Oriol]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> What are the values of dllFileName and dllFileName2 when the assert hits?

I updated my local repository and now I can't see dllFileName and dllFileName2 because an unrelated assert happens before this GFX one. I'm trying to find an older revision, but if I compile with --enable-artifact-builds then I can't debug, and otherwise the compilation is so slow.

So I will confirm later, but if I understand the code correctly,
 - dllFileName is "igd10umd64.dll" for 64-bit Firefox and "igd10umd32.dll" for 32-bit Firefox.
 - dllFileName2 is "igd10iumd64.dll" for 64-bit Firefox and "igd10iumd32.dll" for 32-bit Firefox.

Currently I'm using 64-bit Firefox, so it should be "igd10umd64.dll" and "igd10iumd64.dll".

Comment 8

[Oriol Brufau [:Oriol]]

Yes, dllFileName is "igd10umd64.dll" and dllFileName2 is "igd10iumd64.dll".

(In reply to Milan Sreckovic [:milan] from comment #6)
> Maybe the DLLs don't match the driver version numbers anymore

The problem is that now there is no such DLL with these names, so the version defaults to "0.0.0.0".

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

https://crash-stats.mozilla.com/report/index/d91f46dc-7157-4c34-8de7-d434a2170220#tab-metadata

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks.

What does your about:support graphics look like when you're running the 64-bit Firefox?  The one from comment 0 is for the 32-bit?

Comment 11

[Oriol Brufau [:Oriol]]

I thought the graphics info was the same for 32 and 64 bits. Not sure if the graphics info in comment 0 was from my compiled 32-bit Firefox, or maybe from the 64-bit official Nightly downloaded from Mozilla.

Here is the graphics info from the 64-bit Nightly 2017-02-23 downloaded from Mozilla.
> Features
>  - Compositing:                Direct3D 11
>  - Asynchronous Pan/Zoom:      none
>  - WebGL 1 Renderer:           Google Inc. -- ANGLE (Intel(R) HD Graphics 530 Direct3D11 vs_5_0 ps_5_0)
>  - WebGL 1 GL Version:         OpenGL ES 2.0 (ANGLE 2.1.0.2a250c8a0e15)
>  - WebGL 1 GL Extensions:      GL_ANGLE_depth_texture GL_ANGLE_framebuffer_blit GL_ANGLE_framebuffer_multisample GL_ANGLE_instanced_arrays GL_ANGLE_lossy_etc_decode GL_ANGLE_pack_reverse_row_order GL_ANGLE_robust_client_memory GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_ANGLE_texture_usage GL_ANGLE_translated_shader_source GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_compressed_texture GL_CHROMIUM_copy_texture GL_CHROMIUM_sync_query GL_EXT_blend_minmax GL_EXT_color_buffer_half_float GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_EXT_disjoint_timer_query GL_EXT_draw_buffers GL_EXT_frag_depth GL_EXT_map_buffer_range GL_EXT_occlusion_query_boolean GL_EXT_read_format_bgra GL_EXT_robustness GL_EXT_sRGB GL_EXT_shader_texture_lod GL_EXT_texture_compression_dxt1 GL_EXT_texture_filter_anisotropic GL_EXT_texture_format_BGRA8888 GL_EXT_texture_rg GL_EXT_texture_storage GL_EXT_unpack_subimage GL_KHR_debug GL_NV_EGL_stream_consumer_external GL_NV_fence GL_NV_pack_subimage GL_NV_pixel_buffer_object GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth32 GL_OES_element_index_uint GL_OES_get_program_binary GL_OES_mapbuffer GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_texture_float GL_OES_texture_float_linear GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_array_object 
>  - WebGL 1 WSI Info:
>     - EGL_VENDOR:              Google Inc. (adapter LUID: 000000000000a11d)
>     - EGL_VERSION:             1.4 (ANGLE 2.1.0.2a250c8a0e15)
>     - EGL_EXTENSIONS:          EGL_EXT_create_context_robustness EGL_ANGLE_d3d_share_handle_client_buffer EGL_ANGLE_d3d_texture_client_buffer EGL_ANGLE_surface_d3d_texture_2d_share_handle EGL_ANGLE_query_surface_pointer EGL_ANGLE_window_fixed_size EGL_ANGLE_keyed_mutex EGL_ANGLE_surface_orientation EGL_ANGLE_direct_composition EGL_NV_post_sub_buffer EGL_KHR_create_context EGL_EXT_device_query EGL_KHR_image EGL_KHR_image_base EGL_KHR_gl_texture_2D_image EGL_KHR_gl_texture_cubemap_image EGL_KHR_gl_renderbuffer_image EGL_KHR_get_all_proc_addresses EGL_KHR_stream EGL_KHR_stream_consumer_gltexture EGL_NV_stream_consumer_gltexture_yuv EGL_ANGLE_flexible_surface_compatibility EGL_ANGLE_stream_producer_d3d_texture_nv12 EGL_ANGLE_create_context_webgl_compatibility EGL_CHROMIUM_create_context_bind_generates_resource 
>     - EGL_EXTENSIONS(nullptr): EGL_EXT_client_extensions EGL_EXT_platform_base EGL_EXT_platform_device EGL_ANGLE_platform_angle EGL_ANGLE_platform_angle_d3d EGL_ANGLE_device_creation EGL_ANGLE_device_creation_d3d11 EGL_ANGLE_experimental_present_path EGL_KHR_client_get_all_proc_addresses 
>  - WebGL2 Renderer:            Google Inc. -- ANGLE (Intel(R) HD Graphics 530 Direct3D11 vs_5_0 ps_5_0)
>  - WebGL 2 GL Version:         OpenGL ES 3.0 (ANGLE 2.1.0.2a250c8a0e15)
>  - WebGL 2 GL Extensions:      GL_ANGLE_depth_texture GL_ANGLE_framebuffer_blit GL_ANGLE_framebuffer_multisample GL_ANGLE_instanced_arrays GL_ANGLE_lossy_etc_decode GL_ANGLE_pack_reverse_row_order GL_ANGLE_robust_client_memory GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_ANGLE_texture_usage GL_ANGLE_translated_shader_source GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_compressed_texture GL_CHROMIUM_copy_texture GL_CHROMIUM_sync_query GL_EXT_blend_minmax GL_EXT_color_buffer_float GL_EXT_color_buffer_half_float GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_EXT_disjoint_timer_query GL_EXT_draw_buffers GL_EXT_frag_depth GL_EXT_map_buffer_range GL_EXT_occlusion_query_boolean GL_EXT_read_format_bgra GL_EXT_robustness GL_EXT_sRGB GL_EXT_shader_texture_lod GL_EXT_texture_compression_dxt1 GL_EXT_texture_filter_anisotropic GL_EXT_texture_format_BGRA8888 GL_EXT_texture_norm16 GL_EXT_texture_rg GL_EXT_texture_storage GL_EXT_unpack_subimage GL_KHR_debug GL_NV_EGL_stream_consumer_external GL_NV_fence GL_NV_pack_subimage GL_NV_pixel_buffer_object GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_EGL_image_external_essl3 GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth32 GL_OES_element_index_uint GL_OES_get_program_binary GL_OES_mapbuffer GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_texture_float GL_OES_texture_float_linear GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_array_object 
>  - WebGL 2 WSI Info:
>     - EGL_VENDOR:              Google Inc. (adapter LUID: 000000000000a11d)
>     - EGL_VERSION:             1.4 (ANGLE 2.1.0.2a250c8a0e15)
>     - EGL_EXTENSIONS:          EGL_EXT_create_context_robustness EGL_ANGLE_d3d_share_handle_client_buffer EGL_ANGLE_d3d_texture_client_buffer EGL_ANGLE_surface_d3d_texture_2d_share_handle EGL_ANGLE_query_surface_pointer EGL_ANGLE_window_fixed_size EGL_ANGLE_keyed_mutex EGL_ANGLE_surface_orientation EGL_ANGLE_direct_composition EGL_NV_post_sub_buffer EGL_KHR_create_context EGL_EXT_device_query EGL_KHR_image EGL_KHR_image_base EGL_KHR_gl_texture_2D_image EGL_KHR_gl_texture_cubemap_image EGL_KHR_gl_renderbuffer_image EGL_KHR_get_all_proc_addresses EGL_KHR_stream EGL_KHR_stream_consumer_gltexture EGL_NV_stream_consumer_gltexture_yuv EGL_ANGLE_flexible_surface_compatibility EGL_ANGLE_stream_producer_d3d_texture_nv12 EGL_ANGLE_create_context_webgl_compatibility EGL_CHROMIUM_create_context_bind_generates_resource 
>     - EGL_EXTENSIONS(nullptr): EGL_EXT_client_extensions EGL_EXT_platform_base EGL_EXT_platform_device EGL_ANGLE_platform_angle EGL_ANGLE_platform_angle_d3d EGL_ANGLE_device_creation EGL_ANGLE_device_creation_d3d11 EGL_ANGLE_experimental_present_path EGL_KHR_client_get_all_proc_addresses 
>  - Audio Backend:              wasapi
>  - Direct2D:                   true
>  - DirectWrite:                true (10.0.14393.351)
> GPU #1
>  - Active:                     Yes
>  - Description:                Intel(R) HD Graphics 530
>  - Vendor ID:                  0x8086
>  - Device ID:                  0x1912
>  - Driver Version:             21.20.16.4590
>  - Driver Date:                1-18-2017
>  - Drivers:                    igdumdim64 igd10iumd64 igd10iumd64 igd12umd64 igdumdim32 igd10iumd32 igd10iumd32 igd12umd32
>  - Subsys ID:                  00000000
>  - RAM:                        Unknown
> Diagnostics
>  - AzureCanvasAccelerated:     0
>  - AzureCanvasBackend:         direct2d 1.1
>  - AzureContentBackend:        direct2d 1.1
>  - AzureFallbackCanvasBackend: skia
>  - failures:                   CP+[GFX1]: Potential driver version mismatch ignored due to missing DLLs igd10umd64 v= and igd10iumd64.dll v=0.0.0.0
> Decision Log
>  - D3D9_COMPOSITING:           disabled by default: Disabled by default
>  - GPU_PROCESS:                unavailable by default: Multi-process mode is not enabled
>  - WEBRENDER:                  unavailable by runtime: Build doesn't include WebRender
> Failure Log
>  - (#0):                       CP+[GFX1]: Potential driver version mismatch ignored due to missing DLLs igd10umd64 v= and igd10iumd64.dll v=0.0.0.0

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

OK, this helps a lot!  Thanks for spending all this time on helping us figure it out.

This suggests that igd10umd64.dll is not on the system, but igd10iumd64.dll is, and for some reason we parse the version as 0.0.0.0.  Can you find that DLL on your system?  If so, can you check what the version is?

Comment 13

[Oriol Brufau [:Oriol]]

With driver 15.40.28.4501 I had both
C:\Windows\System32\igd10iumd64.dll
C:\Windows\SysWOW64\igd10iumd32.dll

I think I didn't have igd10umd64.dll nor igd10umd32.dll, but I'm not sure.

Driver 15.45.10.4542 (21.20.16.4542) removed
C:\Windows\SysWOW64\igd10iumd32.dll

But I still had
C:\Windows\System32\igd10iumd64.dll

This produced the assert fail for 32-bit Firefox, but not for 64-bit Firefox.

However, with driver 15.45.14.4590 (21.20.16.4590), I don't have neither.
So presumably now the assert fails both in 32-bit and 64-bit.

However, I have
C:\Windows\System32\DriverStore\FileRepository\igdlh64.inf_amd64_dada2949e7e4cbd9\igd10iumd32.dll
C:\Windows\System32\DriverStore\FileRepository\igdlh64.inf_amd64_dada2949e7e4cbd9\igd10iumd64.dll
C:\Windows\System32\DriverStore\FileRepository\igdlh64.inf_amd64_f54a4c96d3261a9e\igd10iumd64.dll

The version is parsed as 0.0.0.0 because it's the default value.
See https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/gfx/thebes/gfxWindowsPlatform.cpp#978
> void
> gfxWindowsPlatform::GetDLLVersion(char16ptr_t aDLLPath, nsAString& aVersion)
> {
>     DWORD versInfoSize, vers[4] = {0};
>     // version info not available case
>     aVersion.AssignLiteral(u"0.0.0.0");

Comment 14

[Oriol Brufau [:Oriol]]

Attachment: Get full DLL paths from UserModeDriverName instead of InstalledDisplayDrivers

This fixes the problem for me. Instead of getting only the DLL names from InstalledDisplayDrivers, this gets the full paths from UserModeDriverName.

In the registry the separators are nul characters, but when I get the value they become spaces somehow. So I hope no path has a space, because I use them as separators.

Comment 15

[Oriol Brufau [:Oriol]]

Comment on attachment 8843333 [details] [diff] [review]
Get full DLL paths from UserModeDriverName instead of InstalledDisplayDrivers

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

::: widget/windows/GfxInfo.cpp
@@ +724,5 @@
>  
>  NS_IMETHODIMP
> +GfxInfo::GetAdapterDriverPaths(nsAString & aAdapterDriverPaths)
> +{
> +  if (NS_FAILED(GetKeyValue(mDeviceKey[mActiveGPUIndex].get(), L"UserModeDriverName", aAdapterDriverPaths, REG_MULTI_SZ)))

I guess 32-bit Firefox on 64-bit Windows should use UserModeDriverNameWow.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8843333 [details] [diff] [review]
Get full DLL paths from UserModeDriverName instead of InstalledDisplayDrivers

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

Thanks for the patch!  I know it's extra work for you, but would you mind making a version of this that just changes the functionality, without also switching us to a "array of two is nicer than duplicating code" approach.  It's a mixture of functional and arguably cosmetic changes, and most of the time it's better if they are separated.  You can do a split to two patches, first one for functionality, second one for the "array approach" and still end up at the same result, for example.

Comment 17

[Oriol Brufau [:Oriol]]

Attachment: Get full GFX DLL paths from UserModeDriverName as a fallback

Since I worried that entirely replacing InstalledDisplayDrivers with UserModeDriverName might be problematic for some GPUs, this patch only checks UserModeDriverName as a fallback.

I removed this check
> if (NS_SUCCEEDED(GetAdapterDriver(elligibleDLLs)))
because it always returns NS_OK.

I don't know the difference between u" " and L" ".

Comment 18

[Oriol Brufau [:Oriol]]

Attachment: Merge variables into array to be able to use a loop and remove duplicated code

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8843439 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback

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

Nice, thanks for doing this.  There are a few things to clear up, but they are minor.  I'll give it an r- because of a "misleading" change in GfxInfo.h and dealing with spaces in paths, but it's really close.

::: widget/windows/GfxInfo.cpp
@@ +576,5 @@
>               driverNumericVersion = 0, knownSafeMismatchVersion = 0;
>  
>      // Only parse the DLL version for those found in the driver list
> +    nsAutoString elligibleDLLs, elligibleDLLpaths;
> +    GetAdapterDriver(elligibleDLLs);

Why don't we need to check the success of GetAdapterDriver call?  I know it currently always succeeds, but it may not be a bad idea to leave the check in, in case that ever changes.

@@ +587,5 @@
> +        // might provide the full path to the DLL in some DriverStore FileRepository.
> +        if (elligibleDLLpaths.Length() == 0) {
> +          GetAdapterDriverPaths(elligibleDLLpaths);
> +        }
> +        nsAutoString separator(u" ");

This section gets much simpler (and correct, when it comes to paths that had spaces in them in the first place) once the utility function returns an array, rather than a concatenated string.

@@ +745,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +GfxInfo::GetAdapterDriverPaths(nsAString & aAdapterDriverPaths)

This should probably return an array of strings (nsTArray<nsString>) instead of concatenating the individual paths into a single string, then splitting them apart.  It avoids a problem when the path has a space in it.
If this ends up being a file local function, you'd just need to pass in the mDeviceKey[mActiveGPUIndex] to not need it to be a member function.

::: widget/windows/GfxInfo.h
@@ +26,5 @@
>    NS_IMETHOD GetDWriteVersion(nsAString & aDwriteVersion) override;
>    NS_IMETHOD GetCleartypeParameters(nsAString & aCleartypeParams) override;
>    NS_IMETHOD GetAdapterDescription(nsAString & aAdapterDescription) override;
>    NS_IMETHOD GetAdapterDriver(nsAString & aAdapterDriver) override;
> +  NS_IMETHOD GetAdapterDriverPaths(nsAString & aAdapterDriver);

Don't really want it in here, and named this way; it suggests that we are exposing this interface to JS, and that it shows up in nsIGfxInfo.idl.  If it is a member function, you want it called something else, not be public, and be in another section of this file. Since this is a utility function though, it is probably enough to make it local to GfxInfo.cpp and not even make it a member function.

Comment 20

[Oriol Brufau [:Oriol]]

OK, using GetKeyValue was a bad idea.

Comment 21

[Oriol Brufau [:Oriol]]

Attachment: Get full GFX DLL paths from UserModeDriverName as a fallback - v2

This fixes the issues of the previous patch. And there isn't much repeated code, so no need to replace the pairs of variables with arrays.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8844603 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback - v2

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

I still want to review GetAdapterDriverPaths/GetKeyValues functions, but wanted to drop other notes as that may get delayed a bit.

::: widget/windows/GfxInfo.cpp
@@ +636,5 @@
>      }
>  
> +    // Sometimes the DLL is not in the System32 nor SysWOW64 directories. But GetAdapterDriverPaths
> +    // might provide the full path to the DLL in some DriverStore FileRepository.
> +    if (dllNumericVersion == 0 && dllNumericVersion2 == 0) {

This is certainly what we do today, so it should probably stay that way, but I'm thinking of a case where we find the old/bad driver (so, non-zero version) and don't find the new/good driver (so, zero version), and because of the &&, we never check for the version of the good driver.  But, as I said, that changes today's logic, so what you have here is no more wrong than what we had.

@@ +639,5 @@
> +    // might provide the full path to the DLL in some DriverStore FileRepository.
> +    if (dllNumericVersion == 0 && dllNumericVersion2 == 0) {
> +      nsTArray<nsString> elligibleDLLpaths;
> +      if (NS_SUCCEEDED(GetAdapterDriverPaths(mDeviceKey[mActiveGPUIndex].get(), elligibleDLLpaths))) {
> +        unsigned int length = elligibleDLLpaths.Length();

You'll want to use size_t or uint32_t as appropriate rather than unsigned int, for coding style purposes.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8844603 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback - v2

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

A few more thoughts.

::: widget/windows/GfxInfo.cpp
@@ +234,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // A sequence of null-terminated strings, terminated by an empty string (\0)
> +  WCHAR wCharValue[1024];

1024 is probably enough, but do we want to fail if it isn't?

@@ +243,5 @@
> +  if (result == ERROR_SUCCESS && resultType == REG_MULTI_SZ) {
> +    DWORD strLen = dwcbData/sizeof(wCharValue[0]) - 1;
> +    DWORD i = 0;
> +    while (i < strLen) {
> +      nsString value(wCharValue + i);

Can't RegQueryValueExW return a buffer with a non-null terminated string?

@@ +256,5 @@
> +
> +  return retval;
> +}
> +
> +nsresult GetAdapterDriverPaths(const WCHAR* keyLocation, nsTArray<nsString>& aAdapterDriverPaths)

Is nsWindowRegKey class useful here, with the usage similar to https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/win/nsMIMEInfoWin.cpp#658 ?  Not having to do the parsing part in GetKeyValues.

@@ +261,5 @@
> +{
> +  // Get the paths from UserModeDriverName, and also from UserModeDriverNameWow if available.
> +  // UserModeDriverName goes first in order to have the same order as InstalledDisplayDrivers.
> +  nsresult retval = GetKeyValues(keyLocation, L"UserModeDriverName", aAdapterDriverPaths);
> +  GetKeyValues(keyLocation, L"UserModeDriverNameWow", aAdapterDriverPaths);

Do we want to check the return value for both of these?  if the first one succeeds, but the second one fails (or vice versa), what's the right thing to do?

Comment 24

[Oriol Brufau [:Oriol]]

(In reply to Milan Sreckovic [:milan] from comment #23)
> Comment on attachment 8844603 [details] [diff] [review]
> 1024 is probably enough, but do we want to fail if it isn't?

No idea, I just copied that part from GetKeyValue.

> Can't RegQueryValueExW return a buffer with a non-null terminated string?

I read https://msdn.microsoft.com/en-us/library/windows/desktop/ms724884(v=vs.85).aspx, and it says
> A sequence of null-terminated strings, terminated by an empty string (\0).

I tried removing the nulls at the end using regedit, but they are autoinserted again.

So I think it must be null-terminated.

> Is nsWindowRegKey class useful here

Will look into that.

> Do we want to check the return value for both of these?

I think UserModeDriverNameWow is expected to fail in 32-bit Windows. So I considered it optional and returned the result of UserModeDriverName. But maybe ignoring the result of UserModeDriverNameWow in 32-bit Firefox in 64-bit Windows is bad. Better ideas?

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Oriol from comment #24)
> ...
> 
> > Do we want to check the return value for both of these?
> 
> I think UserModeDriverNameWow is expected to fail in 32-bit Windows. So I
> considered it optional and returned the result of UserModeDriverName. But
> maybe ignoring the result of UserModeDriverNameWow in 32-bit Firefox in
> 64-bit Windows is bad. Better ideas?

Having a comment that mentions this is probably enough.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Oriol from comment #24)
> (In reply to Milan Sreckovic [:milan] from comment #23)
> > Comment on attachment 8844603 [details] [diff] [review]
> > 1024 is probably enough, but do we want to fail if it isn't?
> 
> No idea, I just copied that part from GetKeyValue.
> 
> > Can't RegQueryValueExW return a buffer with a non-null terminated string?
> 
> I read
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724884(v=vs.85).
> aspx, and it says
> > A sequence of null-terminated strings, terminated by an empty string (\0).
> 
> I tried removing the nulls at the end using regedit, but they are
> autoinserted again.
> 
> So I think it must be null-terminated.
> 
> > Is nsWindowRegKey class useful here
> 
> Will look into that.

If the above "null-terminated" is guaranteed, we may not need it; it may be OK the way you had it.

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f6e52e936b3dbd41f7c29922e07e07ea308387a

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8844603 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback - v2

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

I think the code to parse the registry is probably unsafe, so r-

::: widget/windows/GfxInfo.cpp
@@ +225,5 @@
> +{
> +  HKEY key;
> +  DWORD dwcbData;
> +  DWORD resultType;
> +  LONG result;

I'd prefer to see these declared at the point of first use, so that they get a value assigned to them right away.

@@ +243,5 @@
> +  if (result == ERROR_SUCCESS && resultType == REG_MULTI_SZ) {
> +    DWORD strLen = dwcbData/sizeof(wCharValue[0]) - 1;
> +    DWORD i = 0;
> +    while (i < strLen) {
> +      nsString value(wCharValue + i);

I think RegQueryValueExW can indeed return a buffer with a non-null-terminated string. It say so in the MSDN docs at [1]: "If the data has the REG_SZ, REG_MULTI_SZ or REG_EXPAND_SZ type, the string may not have been stored with the proper terminating null characters. Therefore, even if the function returns ERROR_SUCCESS, the application should ensure that the string is properly terminated before using it; otherwise, it may overwrite a buffer."

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx

@@ +261,5 @@
> +{
> +  // Get the paths from UserModeDriverName, and also from UserModeDriverNameWow if available.
> +  // UserModeDriverName goes first in order to have the same order as InstalledDisplayDrivers.
> +  nsresult retval = GetKeyValues(keyLocation, L"UserModeDriverName", aAdapterDriverPaths);
> +  GetKeyValues(keyLocation, L"UserModeDriverNameWow", aAdapterDriverPaths);

Based on the calling code it looks like we should return NS_OK if either if the two functions succeeds. Really using nsresult is overkill and we could just use a bool with |.

@@ +639,5 @@
> +    // might provide the full path to the DLL in some DriverStore FileRepository.
> +    if (dllNumericVersion == 0 && dllNumericVersion2 == 0) {
> +      nsTArray<nsString> elligibleDLLpaths;
> +      if (NS_SUCCEEDED(GetAdapterDriverPaths(mDeviceKey[mActiveGPUIndex].get(), elligibleDLLpaths))) {
> +        unsigned int length = elligibleDLLpaths.Length();

s/elligible/eligible/

Comment 31

[Oriol Brufau [:Oriol]]

Attachment: Get full GFX DLL paths from UserModeDriverName as a fallback - v3

This uses RegGetValueW to ensure there is a terminating null character.

By the way, nsWindowRegKey was not useful because it does not support REG_MULTI_SZ, so would still require parsing.

I renamed various occurrences of 'elligible' to 'eligible'.

And GetAdapterDriverPaths wasn't much useful, I have inlined the code.

Do I need to do anything else? Kartikaya, Milan, who wants to review?

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8853630 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback - v3

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

This looks good to me. I'm happy to r+ this. Thanks!

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Let me know if you want me to land the patch for you.

Comment 34

[Oriol Brufau [:Oriol]]

Attachment: Get full GFX DLL paths from UserModeDriverName as a fallback - v4

A buffer size of 1024 was probably enough, I only needed 391. But just in case, this patch firsts asks for the size.

Please land if you approve.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8853679 [details] [diff] [review]
Get full GFX DLL paths from UserModeDriverName as a fallback - v4

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

This needs to delete[] wCharValue to avoid leaking it.

Comment 36

[Oriol Brufau [:Oriol]]

Attachment: Get full GFX DLL paths from UserModeDriverName as a fallback - v4.1

Ah, I forgot. Now with delete.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f079689d79be
Get full GFX DLL paths from UserModeDriverName as a fallback. r=kats

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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