Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(853)

Issue 2078913005: Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not … (Closed)

Created:
4 years, 6 months ago by ananta
Modified:
4 years, 6 months ago
CC:
chromium-reviews, tfarina, caitkp+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 Committed: https://crrev.com/d936bc1b311562fe4055556e37e841355616ae75 Cr-Commit-Position: refs/heads/master@{#401413}

Patch Set 1 #

Patch Set 2 : Don't need to initialize the TestSuite instance in chrome_elf\run_all_unittests.cc #

Patch Set 3 : Fix typo #

Total comments: 14

Patch Set 4 : Fix trybot redness and address comments #

Patch Set 5 : Avoid dependencies on user32 functions in base\process\launch_win.cc #

Patch Set 6 : Add delay loads to chrome_elf_unittests #

Patch Set 7 : Fix windows redness #

Patch Set 8 : Delayload user32 for chrome_elf_unittests in its gyp #

Patch Set 9 : Revert changes to base\launch #

Total comments: 2

Patch Set 10 : Add a static method InitFromArgvForTesting for Windows to honor passed in arguments #

Total comments: 2

Patch Set 11 : Remove newlines #

Patch Set 12 : Restore newline in CommandLine::Init #

Total comments: 2

Patch Set 13 : Remove braces #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -8 lines) Patch
M base/command_line.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments Download
M base/command_line.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -0 lines 0 comments Download
M base/message_loop/message_pump_win.cc View 1 2 3 4 5 6 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/installer/util/google_update_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -5 lines 0 comments Download
M chrome_elf/BUILD.gn View 1 2 3 4 5 6 3 chunks +13 lines, -1 line 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 2 3 4 5 6 7 2 chunks +23 lines, -0 lines 0 comments Download
M chrome_elf/elf_imports_unittest.cc View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
A chrome_elf/run_all_unittests.cc View 1 2 3 4 5 6 7 8 9 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 40 (13 generated)
ananta
scottmg, robertshield please review everything. thestig please review changes to base.
4 years, 6 months ago (2016-06-18 02:29:00 UTC) #2
scottmg
https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#newcode75 base/command_line.h:75: // CommandLineToArgVW to parse the command line and convert ...
4 years, 6 months ago (2016-06-18 17:36:57 UTC) #5
Lei Zhang
The CL that landed last week got reverted, plus the try bots are still red.
4 years, 6 months ago (2016-06-20 16:24:50 UTC) #6
robertshield
https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_unittest.cc File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_unittest.cc#newcode116 chrome_elf/elf_imports_unittest.cc:116: EXPECT_EQ(nullptr, ::GetModuleHandle(L"user32.dll")); Can you expand the comment here to ...
4 years, 6 months ago (2016-06-20 16:59:55 UTC) #7
ananta
On 2016/06/20 16:24:50, Lei Zhang wrote: > The CL that landed last week got reverted, ...
4 years, 6 months ago (2016-06-20 18:50:36 UTC) #8
ananta
https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#newcode75 base/command_line.h:75: // CommandLineToArgVW to parse the command line and convert ...
4 years, 6 months ago (2016-06-20 19:08:04 UTC) #9
Lei Zhang
Is it possible to create a new test launcher that uses CommandLine::InitFromArgv() instead of CommandLine::Init() ...
4 years, 6 months ago (2016-06-20 22:44:24 UTC) #10
ananta
On 2016/06/20 22:44:24, Lei Zhang wrote: > Is it possible to create a new test ...
4 years, 6 months ago (2016-06-20 23:35:17 UTC) #11
ananta
On 2016/06/20 23:35:17, ananta wrote: > On 2016/06/20 22:44:24, Lei Zhang wrote: > > Is ...
4 years, 6 months ago (2016-06-21 02:28:09 UTC) #13
ananta
On 2016/06/21 02:28:09, ananta wrote: > On 2016/06/20 23:35:17, ananta wrote: > > On 2016/06/20 ...
4 years, 6 months ago (2016-06-21 19:09:25 UTC) #15
Lei Zhang
https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/message_pump_win.cc File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/message_pump_win.cc#oldcode125 base/message_loop/message_pump_win.cc:125: InitUser32APIs(); Can you elaborate on why MessagePumpForIO doesn't need ...
4 years, 6 months ago (2016-06-21 20:40:14 UTC) #16
Lei Zhang
On 2016/06/20 23:35:17, ananta wrote: > On 2016/06/20 22:44:24, Lei Zhang wrote: > > Is ...
4 years, 6 months ago (2016-06-21 20:50:50 UTC) #17
ananta
On 2016/06/21 20:50:50, Lei Zhang wrote: > On 2016/06/20 23:35:17, ananta wrote: > > On ...
4 years, 6 months ago (2016-06-21 21:53:32 UTC) #18
ananta
https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/message_pump_win.cc File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/message_pump_win.cc#oldcode125 base/message_loop/message_pump_win.cc:125: InitUser32APIs(); On 2016/06/21 20:40:14, Lei Zhang wrote: > Can ...
4 years, 6 months ago (2016-06-21 21:57:07 UTC) #20
Lei Zhang
Sure, CommandLine::Init() looks cleaner then. https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc#newcode203 base/command_line.cc:203: DCHECK(current_process_commandline_); missing '!'
4 years, 6 months ago (2016-06-21 21:57:45 UTC) #21
Lei Zhang
On 2016/06/21 21:57:07, ananta wrote: > Thanks. I updated the description on the patch set. ...
4 years, 6 months ago (2016-06-21 21:59:24 UTC) #22
ananta
https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc#newcode203 base/command_line.cc:203: DCHECK(current_process_commandline_); On 2016/06/21 21:57:45, Lei Zhang wrote: > missing ...
4 years, 6 months ago (2016-06-21 22:05:16 UTC) #25
ananta
On 2016/06/21 21:59:24, Lei Zhang wrote: > On 2016/06/21 21:57:07, ananta wrote: > > Thanks. ...
4 years, 6 months ago (2016-06-21 22:05:37 UTC) #26
Lei Zhang
lgtm
4 years, 6 months ago (2016-06-21 22:23:34 UTC) #27
scottmg
lgtm
4 years, 6 months ago (2016-06-21 22:47:35 UTC) #28
ananta
+grt for chrome/installer owners
4 years, 6 months ago (2016-06-21 23:38:35 UTC) #30
grt (UTC plus 2)
lgtm https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/google_update_util.cc File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/google_update_util.cc#newcode180 chrome/installer/util/google_update_util.cc:180: if (base::win::UserAccountControlIsEnabled()) { uber nit: omit braces
4 years, 6 months ago (2016-06-22 14:13:55 UTC) #31
ananta
https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/google_update_util.cc File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/google_update_util.cc#newcode180 chrome/installer/util/google_update_util.cc:180: if (base::win::UserAccountControlIsEnabled()) { On 2016/06/22 14:13:55, grt (no reviews ...
4 years, 6 months ago (2016-06-22 18:43:52 UTC) #32
robertshield
lgtm
4 years, 6 months ago (2016-06-22 19:39:24 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078913005/240001
4 years, 6 months ago (2016-06-22 21:00:44 UTC) #36
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 6 months ago (2016-06-22 21:40:54 UTC) #38
commit-bot: I haz the power
4 years, 6 months ago (2016-06-22 21:42:23 UTC) #40
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/d936bc1b311562fe4055556e37e841355616ae75
Cr-Commit-Position: refs/heads/master@{#401413}

Powered by Google App Engine
This is Rietveld 408576698