Workaround for Windows-only crash inside delay load helper.

Workaround for Windows-only crash inside delay load helper.
Crash report shows access violation inside LoadLibraryExA() called from
DelayLoadHelper2(), and we cannot figure out what exactly causes the problem.
In theory it can be caused by Chrome terminating while we are initializing ffmpeg
code, but I am not sure.

Workaround is to call delay load helper to patch import tables during renderer
initialization. We are loading ffmpeg DLL at this moment anyways but do not patch
import tables. Extra overhead is negligeable compared to disk access time.

Also fixed potential problem -- LoadLibraryEx() does not accept relative paths when
used as we are using it. During normal startup we are getting absolute path, but
customer can specify relative path as a command-line flag. Fix is to call Windows API
GetFullPathName() to get rid of relative path.

BUG=110983
.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123865

[enal1, 2012-02-23 01:09:32]

Hi Andrew,

I believe you are the best person to review the change as you already know code
in question.

[scherkus (not reviewing), 2012-02-23 05:22:55]

to be honest I'm not a windows pro so I'm adding cpu@ who might be able to help
out (or at least re-delegate)

also adding dalecurtis@ in case we missed the function name stuff

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc
File media/base/media_win.cc (right):

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:33: return "avcodec-53.dll";
dalecurtis: do these need updated?

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:59: // Fixes import table for DLL. Returns true if
succeed, false otherwise.
so this function doesn't require absolute paths?

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:60: static bool PatchImportTable(const char *dll) {
pointer w/ type: const char* dll

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:86: DWORD full_path_len =
::GetFullPathName(base_path.value().c_str(),
perhaps turn this into a function like GetFullFilePath(...) as it's a little
confusing having this code inlined here

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:126: // LoadLibrary() sometimes AV's when called by
delay load helper when we
what's AV?

[DaleCurtis, 2012-02-23 21:03:07]

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc
File media/base/media_win.cc (right):

https://chromiumcodereview.appspot.com/9450001/diff/1/media/base/media_win.cc...
media/base/media_win.cc:33: return "avcodec-53.dll";
On 2012/02/23 05:22:55, scherkus wrote:
> dalecurtis: do these need updated?

Not since the FFmpeg roll got reverted. Just make sure to rebase before pushing,
CQ/dcommit will stop you in any case.

[cpu_(ooo_6.6-7.5), 2012-02-23 23:58:24]

http://codereview.chromium.org/9450001/diff/1/media/base/media_win.cc
File media/base/media_win.cc (right):

http://codereview.chromium.org/9450001/diff/1/media/base/media_win.cc#newcode53
media/base/media_win.cc:53: return EXCEPTION_EXECUTE_HANDLER;
I don't understand the purpose of the patch. You swallow 3 exceptions we don't
see and let EXCEPTION_ACCESS_VIOLATION_EXEC  go thru so we still crash?

Or is the associated bug incorrect?

[cpu_(ooo_6.6-7.5), 2012-02-23 23:59:39]

the LoadLibraryEx fix is good. You can't use a relative path in that case. I
suggest you do this fix in a different CL.

[enal1, 2012-02-24 00:18:51]

We are crashing deep inside delay load helper, and I don't see why. I have
2 theories:
* memory corruption (likely because all the crash reports I looked at
happened in extensions)
* some form of race condition -- e.g. delay load helper called when process
is terminated

If we have memory corruption we would die anyway (hopefully somebody else
would be on top of stack prior to Windows API call :-) ), but if we have
race condition it can be fixed by forcing delay load to run in the renderer
initialization. Overhead is negligible compared to LoadLibrary() overhead.

Now about catching the exceptions: I catch well-defined ones thrown by
delay loader, when we know everything is in the reasonable state. Chrome
would work fine as long as nobody uses audio. If somebody tries to patch
trunk for the 2nd time, he'll get exactly the same exception, and Chrome
would die -- not worse than what we are having today.

I am not catching random crash after which state of the program is unknown.
The only reasonable action at this situation would be to shut down and send
crash report to Google, and that is exactly what would happen.

I can remove exception catching code, then we would crash even in cases
user would never listen to audio.

Thanks,
Eugene

On Thu, Feb 23, 2012 at 3:58 PM, <cpu@chromium.org> wrote:

>
>
http://codereview.chromium.**org/9450001/diff/1/media/base/**media_win.cc<htt...
> File media/base/media_win.cc (right):
>
> http://codereview.chromium.**org/9450001/diff/1/media/base/**
>
media_win.cc#newcode53<http://codereview.chromium.org/9450001/diff/1/media/base/media_win.cc#newcode53>
> media/base/media_win.cc:53: return EXCEPTION_EXECUTE_HANDLER;
> I don't understand the purpose of the patch. You swallow 3 exceptions we
> don't see and let EXCEPTION_ACCESS_VIOLATION_**EXEC  go thru so we still
> crash?
>
> Or is the associated bug incorrect?
>
>
http://codereview.chromium.**org/9450001/<http://codereview.chromium.org/9450...
>

[rvargas (doing something else), 2012-02-24 19:30:54]

-1: I don't think we should do this.

Even the part about allowing relative paths in the command line looks out of
place: why is this different than any other command line argument? AFAIK nothing
else supports relative paths... command line arguments is not exactly a
user-friendly  interface to begin with.

Unfortunately I don't have a good suggestion about how to tackle the bug, but
maybe add something to figure out what's going on in the first place.

[enal1, 2012-02-24 20:00:26]

I will remove relative path - related changes, but I still think we need
delay load - related.

We have to load ffmpeg DLLs manually during renderer initialization.
Without that system loader can pick up older version of these DLLs
installed by some 3rd party applications into C:\WINDOWS. See
http://crbug.com/35857 for details.

So we are saying to delay load ffmpeg DLLs, and on initialization call
LoadLibraryEx(..., LOAD_WITH_ALTERED_SEARCH_PATH) with full path to DLLs
installed by Chrome. Later, when calling functions from those DLLs, delay
load code sees that DLLs are already in memory and redirects call to them.

Unfortunately, we are seeing crash reports with access violation somewhere
in the delay load code. It only happens in the extension process.

Delay loader cannot generally work in the extension process, and not
because it has no rights to read from disk, there is no read involved.
Maybe some DLLs it depends on are unloaded, or some memory area is
unmapped, or whatever.

I am trying to force delay loader to patch import tables in the renderer
initialization. If that is not a way to go we have to change the installer
to install DLLs into system directory (usually C:\WINDOWS) and that is not
good, we can break somebody else...

Thanks,
Eugene

On Fri, Feb 24, 2012 at 11:30 AM, <rvargas@chromium.org> wrote:

> -1: I don't think we should do this.
>
> Even the part about allowing relative paths in the command line looks out
> of
> place: why is this different than any other command line argument? AFAIK
> nothing
> else supports relative paths... command line arguments is not exactly a
> user-friendly  interface to begin with.
>
> Unfortunately I don't have a good suggestion about how to tackle the bug,
> but
> maybe add something to figure out what's going on in the first place.
>
>
http://codereview.chromium.**org/9450001/<http://codereview.chromium.org/9450...
>

[cpu_(ooo_6.6-7.5), 2012-02-24 20:25:00]

why don't we let
src/chrome/app/hard_error_handler_win.cc

Do its job? afaik this should apply to the extension process.

[enal1, 2012-02-24 20:37:01]

I wanted to delay raising exception till user really uses audio, not emit
it unconditionally. If you think that is bad, fine -- I can remove
exception handler.

In the crash reports we are seeing access violation in the
extension process, not delay load exception ...

Thanks,
Eugene

On Fri, Feb 24, 2012 at 12:25 PM, <cpu@chromium.org> wrote:

> why don't we let
> src/chrome/app/hard_error_**handler_win.cc
>
> Do its job? afaik this should apply to the extension process.
>
>
>
http://codereview.chromium.**org/9450001/<http://codereview.chromium.org/9450...
>

[cpu_(ooo_6.6-7.5), 2012-02-24 23:05:28]

I am still not convinced of the change, seems it is going to mask the actual
issue. 

Since we have several delay loaded dlls and we don't seem to have issues with
them I think the issue is actually with the media libraries.

But it seems a step that can be taken to learn more

I say remove all manner of exception handling and just call the crazy
bind-all-function.

[enal1, 2012-02-27 17:54:22]

On 2012/02/24 23:05:28, cpu wrote:
> I am still not convinced of the change, seems it is going to mask the actual
> issue. 
> 
> Since we have several delay loaded dlls and we don't seem to have issues with
> them I think the issue is actually with the media libraries.
> 
> But it seems a step that can be taken to learn more
> 
> I say remove all manner of exception handling and just call the crazy
> bind-all-function.

Removed exception handling and path normalization. Now just forcing import
tables patch here, hope it will unmask underlying issue (if any).

Thanks,
Eugene

[cpu_(ooo_6.6-7.5), 2012-02-27 18:23:16]

lgtm

[commit-bot: I haz the power, 2012-02-27 18:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/9450001/7001

[rvargas (doing something else), 2012-02-27 19:23:40]

http://codereview.chromium.org/9450001/diff/7001/media/base/media_win.cc
File media/base/media_win.cc (right):

http://codereview.chromium.org/9450001/diff/7001/media/base/media_win.cc#newc...
media/base/media_win.cc:87: // Workaround for http://crbug.com/110983
nit: There should be a TODO to remove this code when more information is
gathered... this should be an experiment, not a workaround.

[enal1, 2012-02-27 19:33:57]

On 2012/02/27 19:23:40, rvargas wrote:
> http://codereview.chromium.org/9450001/diff/7001/media/base/media_win.cc
> File media/base/media_win.cc (right):
> 
>
http://codereview.chromium.org/9450001/diff/7001/media/base/media_win.cc#newc...
> media/base/media_win.cc:87: // Workaround for http://crbug.com/110983
> nit: There should be a TODO to remove this code when more information is
> gathered... this should be an experiment, not a workaround.

Added comment.

[rvargas (doing something else), 2012-02-27 20:51:29]

lgtm

[commit-bot: I haz the power, 2012-02-27 23:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/9450001/11002

[commit-bot: I haz the power, 2012-02-28 02:02:22]

Change committed as 123865