Issue 7031054: Use CHECK_EQ in NetworkChangeNotifierWin::WatchForAddressChange

Issue 7031054: Use CHECK_EQ in NetworkChangeNotifierWin::WatchForAddressChange (Closed)

Created:
9 years, 7 months ago by mmenke

Modified:
9 years, 7 months ago

Reviewers:
commit-bot: I haz the power, willchan no longer on Chromium

CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

Replace CHECK in NetworkChangeNotifierWin::WatchForAddressChange with CHECK_EQ, so on failure, value will appear in minidumps. BUG=69198 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86675

Patch Set 1 #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1 line, -1 line)			Patch
	M	net/base/network_change_notifier_win.cc	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

mmenke

I checked that the value of |ret| is visible while debugging release builds, and it ...

9 years, 7 months ago (2011-05-25 16:31:03 UTC) #1

willchan no longer on Chromium

LGTM I haven't actually looked at the minidump, but does |ret| really not show up? ...

9 years, 7 months ago (2011-05-25 16:36:27 UTC) #2

mmenke

I have looked at the minidumps, and |ret| didn't seem to appear anywhere. I'm new ...

9 years, 7 months ago (2011-05-25 17:00:49 UTC) #3

I have looked at the minidumps, and |ret| didn't seem to appear anywhere.  I'm
new to using minidumps, so I could have been missing something, but I did see
the other variables.  Running off of my own release builds, it also didn't show
up in MSVC.

CHECK_EQ is actually mapped to a function template (Which is why the cast is
needed), so this does actually add a function call.  Locally, this change made
it show up in MSVC release builds.

On 2011/05/25 16:36:27, willchan wrote:
> LGTM
> 
> I haven't actually looked at the minidump, but does |ret| really not show
> up? I'm not sure if this will make it show up, but feel free to do it. I
> think if it's not showing up, you may need to do a function call instead.
> 
> On Wed, May 25, 2011 at 5:31 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > Reviewers: willchan,
> >
> > Message:
> > I checked that the value of |ret| is visible while debugging release
> > builds, and
> > it is, so presumably will be in minidumps as well.
> >
> > The cast is needed because ERROR_IO_PENDING is, bizarrely, a signed long,
> > but
> > the DWORD that NotifyAddrChange returns is unsigned.
> >
> > Description:
> > Replace CHECK in NetworkChangeNotifierWin::WatchForAddressChange
> > with CHECK_EQ, so on failure, value will appear in minidumps.
> >
> > BUG=69198
> > TEST=none
> >
> > Please review this at http://codereview.chromium.org/7031054/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/
> >
> > Affected files:
> >  M     net/base/network_change_notifier_win.cc
> >
> >
> > Index: net/base/network_change_notifier_win.cc
> > ===================================================================
> > --- net/base/network_change_notifier_win.cc     (revision 86320)
> > +++ net/base/network_change_notifier_win.cc     (working copy)
> > @@ -163,7 +163,7 @@
> >  void NetworkChangeNotifierWin::WatchForAddressChange() {
> >   HANDLE handle = NULL;
> >   DWORD ret = NotifyAddrChange(&handle, &addr_overlapped_);
> > -  CHECK(ret == ERROR_IO_PENDING);
> > +  CHECK_EQ(static_cast<DWORD>(ERROR_IO_PENDING), ret);
> >   addr_watcher_.StartWatching(addr_overlapped_.hEvent, this);
> >  }
> >
> >
> >
> >

willchan no longer on Chromium

OK then, news to me :) On Wed, May 25, 2011 at 6:00 PM, <mmenke@chromium.org> ...

9 years, 7 months ago (2011-05-25 17:03:49 UTC) #4

OK then, news to me :)

On Wed, May 25, 2011 at 6:00 PM, <mmenke@chromium.org> wrote:

> I have looked at the minidumps, and |ret| didn't seem to appear anywhere.
>  I'm
> new to using minidumps, so I could have been missing something, but I did
> see
> the other variables.  Running off of my own release builds, it also didn't
> show
> up in MSVC.
>
> CHECK_EQ is actually mapped to a function template (Which is why the cast
> is
> needed), so this does actually add a function call.  Locally, this change
> made
> it show up in MSVC release builds.
>
>
> On 2011/05/25 16:36:27, willchan wrote:
>
>> LGTM
>>
>
>  I haven't actually looked at the minidump, but does |ret| really not show
>> up? I'm not sure if this will make it show up, but feel free to do it. I
>> think if it's not showing up, you may need to do a function call instead.
>>
>
>  On Wed, May 25, 2011 at 5:31 PM, <mailto:mmenke@chromium.org> wrote:
>>
>
>  > Reviewers: willchan,
>> >
>> > Message:
>> > I checked that the value of |ret| is visible while debugging release
>> > builds, and
>> > it is, so presumably will be in minidumps as well.
>> >
>> > The cast is needed because ERROR_IO_PENDING is, bizarrely, a signed
>> long,
>> > but
>> > the DWORD that NotifyAddrChange returns is unsigned.
>> >
>> > Description:
>> > Replace CHECK in NetworkChangeNotifierWin::WatchForAddressChange
>> > with CHECK_EQ, so on failure, value will appear in minidumps.
>> >
>> > BUG=69198
>> > TEST=none
>> >
>> > Please review this at http://codereview.chromium.org/7031054/
>> >
>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>> >
>> > Affected files:
>> >  M     net/base/network_change_notifier_win.cc
>> >
>> >
>> > Index: net/base/network_change_notifier_win.cc
>> > ===================================================================
>> > --- net/base/network_change_notifier_win.cc     (revision 86320)
>> > +++ net/base/network_change_notifier_win.cc     (working copy)
>> > @@ -163,7 +163,7 @@
>> >  void NetworkChangeNotifierWin::WatchForAddressChange() {
>> >   HANDLE handle = NULL;
>> >   DWORD ret = NotifyAddrChange(&handle, &addr_overlapped_);
>> > -  CHECK(ret == ERROR_IO_PENDING);
>> > +  CHECK_EQ(static_cast<DWORD>(ERROR_IO_PENDING), ret);
>> >   addr_watcher_.StartWatching(addr_overlapped_.hEvent, this);
>> >  }
>> >
>> >
>> >
>> >
>>
>
>
>
> http://codereview.chromium.org/7031054/
>

Change committed as 86675

Expand Messages | Collapse Messages