681238 - TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Armen [:armenzg]]

Bug 680818 got fixed and we can now pass the compilation step and reach this make check failure.

See bug 606473 as well.

From:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314087809.1314099252.14141.gz#err0

Running TestDllInterceptor tests
NEXT ERROR TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook
make[4]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test/win'
NEXT ERROR make[4]: *** [check] Error 1
make[3]: *** [check] Error 2
make[3]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test'
make[2]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre'
make[2]: *** [check] Error 2
make[1]: *** [check] Error 2
make[1]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit'
make: *** [check] Error 2
program finished with exit code 2
elapsedTime=301.292000
TinderboxPrint: check<br/>12518/<em class="testfail">1</em>
Unknown Error: command finished with exit code: 2

Comment 1

[Makoto Kato [:m_kato]]

GetVersionExA entry has too complex MOV.  (mov r64, [rel32])

We hook LdrLoadDll, SetWindowLongPtr (SetWindowLong on Win32), and etc, but we don't inject GetVersionEx.  I believe that this isn't good test case for injection.

We should turn off this test or modify test case.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Modifying the test case to the functions we actually hook sounds like a good way to go.

Comment 3

[Armen [:armenzg]]

How long will that take (roughly)? days or more?
Could we disable the test while it gets fixed?

In the next few days I hope to have try support in case fixing the test will be easier that way.

You probably know but just to be explicit, this is the last bug that I need to be fixed before I can show the win64 builds on tbpl.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

CCing the test author.

Comment 5

[Mike Hommey [:glandium]]

The problem is finding a function that can be verified to have been called successfully when hooked (not the hook, the original function).

Comment 6

[Mike Hommey [:glandium]]

We hook:
- GetWindowInfo
- SetWindowLongA/SetWindowLongPtrA
- SetWindowLongW/SetWindowLongPtrW
- TrackPopupMenu
- LdrLoadDll

None of these seem to fit comment 5, except the first one, which requires a window...

Comment 7

[Mike Hommey [:glandium]]

Attachment: Try intercepting a function inside the executable in TestDllInterceptor

This works for me, but merely by luck. I doubt it works on x64. We'd have to find the right contents for the fillPreload function such that the generated code is supported by the interceptor code. We could go as far as actually writing well-suited assembly.

Comment 8

[Mike Hommey [:glandium]]

Attachment: part 0 - Support a few more opcodes and fix x86 mov r, [r] test

My x86 machine code is a bit rusty, but this should be correct.

Mainly, I needed sub r, imm8 and mov r, [r+imm8] on x86 ; rex.wr and mov r, [r] on x64. These are the most common instructions I was getting when trying various functions for the test case, with or without optimization.
The x64 part could probably be refactored further, and share with x86, but I didn't feel like going much further.

I also spotted a bug in the test for mov r, [r] on x86, that tested REG instead of R/M to filter out the [SIB] and disp32  cases that take more bytes.

Comment 9

[Mike Hommey [:glandium]]

Attachment: part 1 - Intercept a function from the executable in TestDllInterceptor

This makes the test hook a custom function instead of GetVersionEx. It's using UINT64 values mainly because the x64 dll interceptor doesn't support mov with 32 bit registers.
I tested with MSVC8 on x86 and x64, with and without optimization in both cases, and it passed.

Comment 10

[Mike Hommey [:glandium]]

Attachment: part 2 - Test WindowsDllInterceptor on functions we intercept in our codebase

This adds a test to verify that we can indeed hook the functions that our codebase hooks and that are listed in comment 6.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 555704 [details] [diff] [review]
part 0 - Support a few more opcodes and fix x86 mov r, [r] test

I'm going to assume the asm decoding is correct :)

Comment 12

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/76e73aad0fab
http://hg.mozilla.org/integration/mozilla-inbound/rev/be03169f5635
http://hg.mozilla.org/integration/mozilla-inbound/rev/1aed3d723632

Comment 13

[Mike Hommey [:glandium]]

Backed out, because it traded the win64 orange with a win debug one :(
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f7ccdc68e3f

Comment 14

[Mike Hommey [:glandium]]

So, what is happening on win debug is that the function GetProcAddress returns is actually the ILT (import lookup table) entry for the function, and that's only a jump to the real function. We don't support jumps in the interceptor on x86. This is due to -DEBUG turning off -OPT:REF, which actually gets rid of these. So building the executable with -DEBUG -OPT:REF works, but this doesn't sound very robust: there's no guarantee that -OPT:REF will continue to reliably remove these ILT entries. The other option is to add support for jumps in the interceptor.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Addition to part 0

This adds support for jumps on x86

Comment 16

[Ed Morley [:emorley]]

M-i merge of landing:
http://hg.mozilla.org/mozilla-central/rev/76e73aad0fab
http://hg.mozilla.org/mozilla-central/rev/be03169f5635
http://hg.mozilla.org/mozilla-central/rev/1aed3d723632

M-i merge of subsequent backout:
http://hg.mozilla.org/mozilla-central/rev/9f7ccdc68e3f

Comment 17

[Armen [:armenzg]]

I pushed the change to try to see how it behaves:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8

For comparison I will look at:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab

Comment 18

[Armen [:armenzg]]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #17)
> I pushed the change to try to see how it behaves:
> http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8
> 
> For comparison I will look at:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab

The results are satisfactory.

Comment 19

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5dda9668493b
http://hg.mozilla.org/integration/mozilla-inbound/rev/38bbf1e8ae5d
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce43a8644bc0

Comment 20

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/5dda9668493b
http://hg.mozilla.org/mozilla-central/rev/38bbf1e8ae5d
http://hg.mozilla.org/mozilla-central/rev/ce43a8644bc0