Last Comment Bug 681238 - TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook
: TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows Server 2008
: -- normal (vote)
: mozilla9
Assigned To: Mike Hommey [:glandium]
:
Mentors:
: 653361 (view as bug list)
Depends on: 972671
Blocks: tracking_win64 support-win64
  Show dependency treegraph
 
Reported: 2011-08-23 06:17 PDT by Armen Zambrano [:armenzg] (EDT/UTC-4)
Modified: 2014-02-13 17:44 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Try intercepting a function inside the executable in TestDllInterceptor (4.08 KB, patch)
2011-08-24 08:52 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 0 - Support a few more opcodes and fix x86 mov r, [r] test (4.40 KB, patch)
2011-08-25 05:07 PDT, Mike Hommey [:glandium]
vladimir: review+
Details | Diff | Splinter Review
part 1 - Intercept a function from the executable in TestDllInterceptor (4.25 KB, patch)
2011-08-25 05:09 PDT, Mike Hommey [:glandium]
vladimir: review+
Details | Diff | Splinter Review
part 2 - Test WindowsDllInterceptor on functions we intercept in our codebase (2.13 KB, patch)
2011-08-25 05:11 PDT, Mike Hommey [:glandium]
vladimir: review+
Details | Diff | Splinter Review
Addition to part 0 (2.93 KB, patch)
2011-08-30 03:09 PDT, Mike Hommey [:glandium]
vladimir: review+
Details | Diff | Splinter Review

Description Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-08-23 06:17:05 PDT
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] 2011-08-23 20:07:56 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-24 04:55:00 PDT
Modifying the test case to the functions we actually hook sounds like a good way to go.
Comment 3 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-08-24 06:49:28 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-24 06:51:43 PDT
CCing the test author.
Comment 5 Mike Hommey [:glandium] 2011-08-24 06:59:24 PDT
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] 2011-08-24 07:03:36 PDT
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] 2011-08-24 08:52:12 PDT
Created attachment 555423 [details] [diff] [review]
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] 2011-08-25 05:07:04 PDT
Created attachment 555704 [details] [diff] [review]
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] 2011-08-25 05:09:47 PDT
Created attachment 555705 [details] [diff] [review]
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] 2011-08-25 05:11:39 PDT
Created attachment 555706 [details] [diff] [review]
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] 2011-08-26 14:41:49 PDT
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 13 Mike Hommey [:glandium] 2011-08-29 10:51:25 PDT
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] 2011-08-30 02:30:42 PDT
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] 2011-08-30 03:09:09 PDT
Created attachment 556790 [details] [diff] [review]
Addition to part 0

This adds support for jumps on x86
Comment 17 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-08-31 11:44:35 PDT
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 Zambrano [:armenzg] (EDT/UTC-4) 2011-09-01 07:03:52 PDT
(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 21 Makoto Kato [:m_kato] 2011-09-05 04:06:59 PDT
*** Bug 653361 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.