In order to test bug 1435816, we'll need to launch a separate process that tries to inject a DLL into Firefox, confirming that it was blocked. This, being the first InjectEject test, may establish techniques to be used in eventual InjectEject-style tests. Two techniques being considered: 1. gtest, which would launch the separate injection app 2. xpcshell test, borrowing techniques from the Updater tests

Comment on attachment 8987461 [details] Bug 1443411: Add gtests for blocking threads with LoadLibrary start address; https://reviewboard.mozilla.org/r/252700/#review260178 Mostly nits, but I'd like to see it one more time before r+. ::: mozglue/build/WindowsDllBlocklist.h:12 (Diff revision 1) > #define mozilla_windowsdllblocklist_h > > #if (defined(_MSC_VER) || defined(__MINGW32__)) && (defined(_M_IX86) || defined(_M_X64)) > > #include <windows.h> > +#ifdef ENABLE_TESTS Where is this macro set? By the gtest harness? ::: mozglue/build/WindowsDllBlocklist.cpp:412 (Diff revision 1) > + gCreateThreadHook(aWasAllowed, aStartAddress); > + } > +} > + > +#else // ENABLE_TESTS > +#define CallDllLoadHook(args...) I think these macros should just be: #define CallDllLoadHook(...) (no args necessary) ::: mozglue/tests/gtest/Injector/Injector.cpp:29 (Diff revision 1) > +#else > + void* startAddr = (void*)strtoul(argv[2], 0, 0); > + void* threadParam = (void*)strtoul(argv[3], 0, 0); > +#endif > + HANDLE targetProc = OpenProcess(PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION | > + PROCESS_VM_OPERATION | PROCESS_VM_WRITE |PROCESS_VM_READ, Nit: space between | and PROCESS_VM_READ ::: mozglue/tests/gtest/TestDLLEject.cpp:29 (Diff revision 1) > +#define DLL_LEAF_NAME (u"InjectorDLL.dll") > + > +static nsString > +makeString(PUNICODE_STRING aOther) > +{ > + size_t chars = aOther->Length / 2; Please rename to numChars and s/2/sizeof(WCHAR) please ::: mozglue/tests/gtest/TestDLLEject.cpp:61 (Diff revision 1) > + SetEvent(sThreadWasAllowed); > + } > + } > +} > + > +template<typename TgetArgsProc> Can you please add some comments at the beginning of major blocks in this function to describe how it works? That would be helpful to a reader who is less familiar with what we're trying to test here than you or me. ::: mozglue/tests/gtest/TestDLLEject.cpp:65 (Diff revision 1) > + > +template<typename TgetArgsProc> > +static void > +DoTest_CreateRemoteThread_LoadLibrary(TgetArgsProc aGetArgsProc) > +{ > + sTestRunning ++; Style nit: no space before increment/decrement ::: mozglue/tests/gtest/TestDLLEject.cpp:96 (Diff revision 1) > + STARTUPINFOW si = { 0 }; > + si.cb = sizeof(si); > + ::GetStartupInfoW(&si); > + PROCESS_INFORMATION pi = { 0 }; > + > + nsString path = nsString(u"Injector.exe"); Just use nsString path(u"Injector.exe"); ::: mozglue/tests/gtest/TestDLLEject.cpp:98 (Diff revision 1) > + ::GetStartupInfoW(&si); > + PROCESS_INFORMATION pi = { 0 }; > + > + nsString path = nsString(u"Injector.exe"); > + nsString dllPath = nsString(DLL_LEAF_NAME); > + // { I assume this comment block is vesigial? Please remove. ::: mozglue/tests/gtest/TestDLLEject.cpp:146 (Diff revision 1) > + sThreadWasBlocked, > + sThreadWasAllowed, > + sDllWasLoaded, > + pi.hProcess > + }; > + int handleCount = ARRAYSIZE(handles); Please use mozilla::ArrayLength instead (via mozilla/ArrayUtils.h) ::: mozglue/tests/gtest/TestDLLEject.cpp:153 (Diff revision 1) > + > + while(keepGoing) { > + switch(WaitForMultipleObjectsEx(handleCount, handles, > + FALSE, sTimeoutMS, FALSE)) { > + case WAIT_OBJECT_0: { > + EXPECT_TRUE("Thread was blocked successfully."); Nit: indentation ::: mozglue/tests/gtest/TestDLLEject.cpp:198 (Diff revision 1) > + } > + } > + > + // Double-check that injectordll is not loaded. > + auto hExisting = GetModuleHandleW(dllPath.get()); > + EXPECT_TRUE(hExisting == 0 || hExisting == INVALID_HANDLE_VALUE); You don't need to check for INVALID_HANDLE_VALUE with HMODULEs.

Comment on attachment 8987461 [details] Bug 1443411: Add gtests for blocking threads with LoadLibrary start address; https://reviewboard.mozilla.org/r/252700/#review260178 > Where is this macro set? By the gtest harness? This is set by the build config (ac_add_options --disable-tests -> OBJ/mozilla-config.h) documented here https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options. I tested that this config works locally, though I just assume builds-for-release disable this macro.

Comment on attachment 8987461 [details] Bug 1443411: Add gtests for blocking threads with LoadLibrary start address; https://reviewboard.mozilla.org/r/252700/#review260178 New revision addresses all the issues you found, and simplifies the control flow.

Comment on attachment 8987461 [details] Bug 1443411: Add gtests for blocking threads with LoadLibrary start address; https://reviewboard.mozilla.org/r/252700/#review262702 Great, thanks!

Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/130e3b2dd716 Add gtests for blocking threads with LoadLibrary start address;r=aklotz