Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3201009:
Fix gtest warnings when running the process_util_unittest. (Closed)
|
Created:
10 years, 4 months ago by levin Modified:
9 years, 7 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionFix gtest warnings when running the process_util_unittest.
When I run them on OSX, I get a gtest warning about multiple threads:
"Death tests use fork(), which is unsafe particularly in a threaded context.
For this test, Google Test detected 2 threads."
This patch fixes the warnings.
BUG=43165
TEST=base_unittest --gtest_filter=OutOfMemoryDeathTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57118
Patch Set 1 #
Total comments: 1
Messages
Total messages: 9 (0 generated)
This is bug 43165, or at least related. Does the suggested fix in the bug report not work? If not, I'll be ok with your fix. http://codereview.chromium.org/3201009/diff/1/2 File base/process_util_unittest.cc (right): http://codereview.chromium.org/3201009/diff/1/2#newcode594 base/process_util_unittest.cc:594: SetUpInDeathAssert(); Is there a reason to put these inside the macro call?
On Mon, Aug 23, 2010 at 2:52 PM, <vandebo@chromium.org> wrote: > This is bug 43165, or at least related. I didn't realize there was a bug already for this. > Does the suggested fix in the bug > report not work? If not, I'll be ok with your fix. > That may work but it sounds pretty risky and like it should be the option of last resort, see http://code.google.com/p/googletest/wiki/V1_5_0_AdvancedGuide#Death_Tests_And... > http://codereview.chromium.org/3201009/diff/1/2 > File base/process_util_unittest.cc (right): > > http://codereview.chromium.org/3201009/diff/1/2#newcode594 > base/process_util_unittest.cc:594: SetUpInDeathAssert(); > Is there a reason to put these inside the macro call? Yes, it is the function call that creates the extra thread which causes the warning. (I tried to explain this in my addition to the comment. Was it badly worded?) > > > http://codereview.chromium.org/3201009/show >
On 2010/08/23 21:58:19, levin wrote: > On Mon, Aug 23, 2010 at 2:52 PM, <mailto:vandebo@chromium.org> wrote: > > This is bug 43165, or at least related. > I didn't realize there was a bug already for this. Looking at it closer - the Linux problem and the problem you are solving are different, so this change doesn't affect bug 43165. > > Does the suggested fix in the bug > > report not work? If not, I'll be ok with your fix. > > > > That may work but it sounds pretty risky and like it should be the option of > last resort, see > http://code.google.com/p/googletest/wiki/V1_5_0_AdvancedGuide#Death_Tests_And... > > > > http://codereview.chromium.org/3201009/diff/1/2 > > File base/process_util_unittest.cc (right): > > > > http://codereview.chromium.org/3201009/diff/1/2#newcode594 > > base/process_util_unittest.cc:594: SetUpInDeathAssert(); > > Is there a reason to put these inside the macro call? > > > Yes, it is the function call that creates the extra thread which causes the > warning. (I tried to explain this in my addition to the comment. Was it > badly worded?) No, the comment is fine, I was just confusing it with the Linux problem, which is that it can't detect that there are no other threads. LGTM
For my own edification, what in EnableTerminationOnOutOfMemory creates a new thread, I didn't see it. On Mon, Aug 23, 2010 at 3:16 PM, <vandebo@chromium.org> wrote: > On 2010/08/23 21:58:19, levin wrote: > > On Mon, Aug 23, 2010 at 2:52 PM, <mailto:vandebo@chromium.org> wrote: >> > This is bug 43165, or at least related. >> I didn't realize there was a bug already for this. >> > > Looking at it closer - the Linux problem and the problem you are solving > are > different, so this change doesn't affect bug 43165. > > > > Does the suggested fix in the bug >> > report not work? If not, I'll be ok with your fix. >> > >> > > That may work but it sounds pretty risky and like it should be the option >> of >> last resort, see >> > > > http://code.google.com/p/googletest/wiki/V1_5_0_AdvancedGuide#Death_Tests_And... > > > > http://codereview.chromium.org/3201009/diff/1/2 >> > File base/process_util_unittest.cc (right): >> > >> > http://codereview.chromium.org/3201009/diff/1/2#newcode594 >> > base/process_util_unittest.cc:594: SetUpInDeathAssert(); >> > Is there a reason to put these inside the macro call? >> > > > Yes, it is the function call that creates the extra thread which causes >> the >> warning. (I tried to explain this in my addition to the comment. Was it >> badly worded?) >> > > No, the comment is fine, I was just confusing it with the Linux problem, > which > is that it can't detect that there are no other threads. > > > LGTM > > > http://codereview.chromium.org/3201009/show >
Note that this patch addresses two things: 1. It changes the name to *DeathTest which ensures that it runs before other tests which may create threads (and thus trigger this warning), so it may fix the linux issue. This change alone did not fix the warning on OSX. 2. On OSX, I had to move that function call inside the macro. What in the OSX version of EnableTerminationOnOutOfMemory() (in process_util_mac.mm) causes a new thread? I honestly don't know. It is a fairly complex function so I guess that it must be causing the new thread. When I moved the function call to be inside of the macro, the warning stopped, so I realized that my guess must have been correct. On Mon, Aug 23, 2010 at 3:18 PM, Steve VanDeBogart <vandebo@chromium.org>wrote: > For my own edification, what in EnableTerminationOnOutOfMemory creates a > new thread, I didn't see it. > > > On Mon, Aug 23, 2010 at 3:16 PM, <vandebo@chromium.org> wrote: > >> On 2010/08/23 21:58:19, levin wrote: >> >> On Mon, Aug 23, 2010 at 2:52 PM, <mailto:vandebo@chromium.org> wrote: >>> > This is bug 43165, or at least related. >>> I didn't realize there was a bug already for this. >>> >> >> Looking at it closer - the Linux problem and the problem you are solving >> are >> different, so this change doesn't affect bug 43165. >> >> >> > Does the suggested fix in the bug >>> > report not work? If not, I'll be ok with your fix. >>> > >>> >> >> That may work but it sounds pretty risky and like it should be the option >>> of >>> last resort, see >>> >> >> >> http://code.google.com/p/googletest/wiki/V1_5_0_AdvancedGuide#Death_Tests_And... >> >> >> > http://codereview.chromium.org/3201009/diff/1/2 >>> > File base/process_util_unittest.cc (right): >>> > >>> > http://codereview.chromium.org/3201009/diff/1/2#newcode594 >>> > base/process_util_unittest.cc:594: SetUpInDeathAssert(); >>> > Is there a reason to put these inside the macro call? >>> >> >> >> Yes, it is the function call that creates the extra thread which causes >>> the >>> warning. (I tried to explain this in my addition to the comment. Was it >>> badly worded?) >>> >> >> No, the comment is fine, I was just confusing it with the Linux problem, >> which >> is that it can't detect that there are no other threads. >> >> >> LGTM >> >> >> http://codereview.chromium.org/3201009/show >> > >
> What in the OSX version of EnableTerminationOnOutOfMemory() (in > process_util_mac.mm) causes a new thread? I wrote that function, and I don't know either. It certainly shouldn't be doing _any_ thread stuff; it does some pretty scary stuff and requires things to stay single-threaded until it's done.
On Mon, Aug 23, 2010 at 4:34 PM, <avi@chromium.org> wrote: > What in the OSX version of EnableTerminationOnOutOfMemory() (in >> process_util_mac.mm) causes a new thread? >> > > I wrote that function, and I don't know either. It certainly shouldn't be > doing > _any_ thread stuff; it does some pretty scary stuff and requires things to > stay > single-threaded until it's done. It must be doing something that causes the OS to create another thread on 10.6 (which is why gtest issues the warning that 2 threads are running). > > > > > http://codereview.chromium.org/3201009/show >
On 2010/08/23 23:37:49, levin wrote: > It must be doing something that causes the OS to create another thread on > 10.6 You are indeed correct. Investigating... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
