278328 - Dynamic Library loading failures aren't handled gracefully on Windows (DLL load error shows dialog)

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Alex Sirota (iosart)]

The PR_LoadLibrary function calls the Win32 LoadLibrary function on Windows. If
the file that is loaded is invalid somehow, an ugly Windows error popup is shown
to the user, saying something like:
---------------------
XPCOM: EventReceiver: firefox.exe – Bad Image

The application or DLL XXX is not a valid Windows image. Please check this
against your installation diskette.
---------------------
Right now, this dialog is shown by Windows inside the LoadLibrary function if
the target DLL is not a valid Windows DLL file.

Proposed resolution: 
There is a Win32 API function - SetErrorMode - that can be used to control the
Windows behavior when encountering the above error and to prevent the error
popup from being shown. The code can do something like this:

UINT prevErrorMode = ::SetErrorMode(SEM_FAILCRITICALERRORS);
HINSTANCE h = ::LoadLibrary("ImAnInvalidDLLFile");
::SetErrorMode(prevErrorMode);

The above LoadLibrary call will fail silently returning NULL.

The importance of this issue is with extensions and third party plugins - when
there are packaging/installation problems that cause invalid files to end up in
the component directory, the user is presented with a non-informative and
non-user friendly Windows dialog.

Comment 1

[Wan-Teh Chang]

Thanks for the suggestion.  The problem with
the SetErrorMode solution is that error mode
is a process property, and in general a library
should not modify a process property.

Comment 2

[Alex Sirota (iosart)]

(In reply to comment #1)
> Thanks for the suggestion.  The problem with
> the SetErrorMode solution is that error mode
> is a process property, and in general a library
> should not modify a process property.

Well, the process state isn't altered in any way, as it is immediately restored
to the previous value. In any case the DLL loading is typically handled on
startup, so when the process is up and everything's loaded, no more changes
(even temporary) are made. One more thing - I believe it's a good idea to turn
this off globally for the Mozilla products. These dialogs are almost never
informative. I believe that MFC turns them off by default, for example. 

Comment 3

[Wan-Teh Chang]

> the DLL loading is typically handled on startup...

This is true of some applications, but NSPR can't assume
that all applications do this.  Another problem is that
the setting and restoring of error code is not atomic,
so if two threads call PR_LoadLibrary at the same time
and the four SetErrorMode calls are interleaved, the
final error state may not be the original one.

This is a decision that is best made by the application.
I agree all the Mozilla products should turn this off
globally.

Comment 4

[Alex Sirota (iosart)]

> Another problem is that
> the setting and restoring of error code is not atomic,
> so if two threads call PR_LoadLibrary at the same time
> and the four SetErrorMode calls are interleaved, the
> final error state may not be the original one.
I believe that atomic operations can be achieved in NSPR using locks.

> This is a decision that is best made by the application.
> I agree all the Mozilla products should turn this off
> globally.
Can you suggest a new product/component for this bug?

Comment 5

[Wan-Teh Chang]

NSPR locks only help if all callers of SetErrorMode
use the lock.  This requirement is impossible to
enforce in general.

Regarding products, I'm afraid that we will need to
file one bug each for Mozilla Application Suite,
Firefox, Thunderbird, etc.  Perhaps there is some
component of the "Core" product that applies to
all the Mozilla client applications?

Comment 6

[timeless]

note that we can't do wrappings. personally i'd resolve this as a dupe of bug
153377 since the fix and problem cause are the same.

Comment 7

[Wan-Teh Chang]

*** Bug 281471 has been marked as a duplicate of this bug. ***

Comment 8

[Wan-Teh Chang]

Boris, could you advise on how to file a bug to
have all Mozilla-based clients call

    SetErrorMode(SEM_FAILCRITICALERRORS);

on Windows to disable the useless error popup
dialogs?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... we'd probably need one bug per module that has a PR_LoadLibrary call,
really... :(

Comment 10

[Benjamin Smedberg]

I doubt it. We just need a call to SetErrorMode in the two nsAppRunner.cpp
files, and maybe mfcembed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, if we just want to globally disable it?  Sure.

Comment 12

[Wan-Teh Chang]

Yes, we want to globally disable it in every Mozilla
application (suite, browser, mail, etc.).

My question is whether we need to file one bug each
for Mozilla Application Suite, Firefox, Thunderbird,
etc., or just a single bug for Core.

We need to call

    SetErrorMode(SEM_FAILCRITICALERRORS);

at the beginning of the main() function of these
applications.

Comment 13

[Alex Sirota (iosart)]

(In reply to comment #12)
> We need to call
> 
>     SetErrorMode(SEM_FAILCRITICALERRORS);
> 
> at the beginning of the main() function of these
> applications.

Wouldn't it be better to add this call to the beginning of the xre_main()
function? This way we won't have to modify every XUL based application out there. 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, this sounds like a Toolkit:XRE Startup issue....

Comment 15

[Wan-Teh Chang]

Alex -- do you have enough information to file
a bug (against Toolkit: XRE Startup)?  In that
bug please refer to bug 153377 and this bug.

Comment 16

[Alex Sirota (iosart)]

Filed bug 285497 against Toolkit: XRE Startup.