Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8588061:
suppress unit_tests deliberate null deref (Closed)
|
Created:
9 years, 1 month ago by Derek Bruening Modified:
9 years ago Reviewers:
Timur Iskhodzhanov CC:
chromium-reviews, Alexander Potapenko, pam+watch_chromium.org, stuartmorgan+watch_chromium.org, Joao da Silva, Reid Kleckner (google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionsuppress unit_tests deliberate null deref
TBR=timurrrr
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110720
Patch Set 1 #
Total comments: 6
Messages
Total messages: 9 (0 generated)
You've forgotten to specify a reviewer in the rietveld. (if you did upload --send_mail, see the "-r" option) Maybe we should just exclude the tests instead? http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... File tools/valgrind/drmemory/suppressions.txt (right): http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:718: LEAK can you add name= lines to these suppressions?
http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... File tools/valgrind/drmemory/suppressions.txt (right): http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:725: unit_tests.exe!BrowserAboutHandlerTest_WillHandleBrowserAboutURL_Test::TestBody Do you remember what was the stack? This one matches the reports in http://crbug.com/96010 which is a different issue
Reid please see comments http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... File tools/valgrind/drmemory/suppressions.txt (right): http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:718: LEAK On 2011/11/18 20:05:40, Timur Iskhodzhanov wrote: > can you add name= lines to these suppressions? meant to do this next time I changed this file but hasn't happened yet. Reid if you touch this file before me can you add names. http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:725: unit_tests.exe!BrowserAboutHandlerTest_WillHandleBrowserAboutURL_Test::TestBody On 2011/12/07 13:32:16, Timur Iskhodzhanov wrote: > Do you remember what was the stack? > This one matches the reports in http://crbug.com/96010 which is a different > issue This diff came straight from Reid and I just posted it as he didn't have commit access at the time. Reid did you analyze this test and see a "death test"? b/c 96010 is a real bug not a "death test" it seems.
http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... File tools/valgrind/drmemory/suppressions.txt (right): http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:718: LEAK I already did that. On 2011/12/07 14:53:59, Derek Bruening wrote: > On 2011/11/18 20:05:40, Timur Iskhodzhanov wrote: > > can you add name= lines to these suppressions? > > meant to do this next time I changed this file but hasn't happened yet. Reid if > you touch this file before me can you add names.
http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... File tools/valgrind/drmemory/suppressions.txt (right): http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... tools/valgrind/drmemory/suppressions.txt:725: unit_tests.exe!BrowserAboutHandlerTest_WillHandleBrowserAboutURL_Test::TestBody On 2011/12/07 14:53:59, Derek Bruening wrote: > On 2011/12/07 13:32:16, Timur Iskhodzhanov wrote: > > Do you remember what was the stack? > > This one matches the reports in http://crbug.com/96010 which is a different > > issue > > This diff came straight from Reid and I just posted it as he didn't have commit > access at the time. Right: http://codereview.chromium.org/8585046/ > Reid did you analyze this test and see a "death test"? b/c 96010 is a real bug > not a "death test" it seems. The last line of the test is: EXPECT_DEATH(HandleNonNavigationAboutURL(url), ""); Unfortunately I didn't link to any of the failing builds I was looking at at the time.
Can you please try to make the suppression more strict? E.g. one more frame at the top. Now we disable pretty much all the unaddrs in this test, which makes it a bit harder to debug 96010 On 2011/12/07 15:04:22, Reid Kleckner wrote: > http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... > File tools/valgrind/drmemory/suppressions.txt (right): > > http://codereview.chromium.org/8588061/diff/1/tools/valgrind/drmemory/suppres... > tools/valgrind/drmemory/suppressions.txt:725: > unit_tests.exe!BrowserAboutHandlerTest_WillHandleBrowserAboutURL_Test::TestBody > On 2011/12/07 14:53:59, Derek Bruening wrote: > > On 2011/12/07 13:32:16, Timur Iskhodzhanov wrote: > > > Do you remember what was the stack? > > > This one matches the reports in http://crbug.com/96010 which is a different > > > issue > > > > This diff came straight from Reid and I just posted it as he didn't have > commit > > access at the time. > > Right: > http://codereview.chromium.org/8585046/ > > > Reid did you analyze this test and see a "death test"? b/c 96010 is a real > bug > > not a "death test" it seems. > > The last line of the test is: > EXPECT_DEATH(HandleNonNavigationAboutURL(url), ""); > > Unfortunately I didn't link to any of the failing builds I was looking at at the > time.
On Thu, Dec 8, 2011 at 5:47 AM, <timurrrr@chromium.org> wrote: > Can you please try to make the suppression more strict? E.g. one more frame > at > the top. > Now we disable pretty much all the unaddrs in this test, which makes it a > bit > harder to debug 96010 This test hangs for me locally, so I can't actually disable the suppression and reproduce locally to get the full stack trace. Reid
On Thu, Dec 8, 2011 at 2:48 PM, Reid Kleckner <rnk@google.com> wrote: > On Thu, Dec 8, 2011 at 5:47 AM, <timurrrr@chromium.org> wrote: >> Can you please try to make the suppression more strict? E.g. one more frame >> at >> the top. >> Now we disable pretty much all the unaddrs in this test, which makes it a >> bit >> harder to debug 96010 > > This test hangs for me locally, so I can't actually disable the > suppression and reproduce locally to get the full stack trace. I take it back, here's the unaddr I get: UNADDRESSABLE ACCESS: reading 0x00000000-0x00000004 4 byte(s) # 0 TestingProfile::FinishInit [c:\src\src\chome\test\base\testing_profile.cc:210] # 1 TestingProfile::TestingProfile [c:\src\src\chrome\test\base\testing_profile.cc:164] # 2 BrowserAboutHandlerTest_WillHandleBrowserAboutURL_Test::TestBody [c:\src\src\chrome\browser\browser_about_handler_unittest.cc:110] # 3 testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [c:\src\src\testing\gtest\src\gtest.cc:2145] Note: @0:00:29.815 in thread 10328 Note: instruction: mov (%edx) -> %eax Note the line is: src\chrome\browser\browser_about_handler_unittest.cc:110 Which is: TestingProfile profile; And not the EXPECT_DEATH line. This may actually be a DrMemory bug here, because the test eventually succeeds. Reid |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
