Last Comment Bug 712243 - cleanup nsNotifyAddrListener
: cleanup nsNotifyAddrListener
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla13
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 02:35 PST by Makoto Kato [:m_kato]
Modified: 2012-02-15 09:19 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (5.84 KB, patch)
2011-12-20 02:55 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v1.1 (6.33 KB, patch)
2011-12-20 02:58 PST, Makoto Kato [:m_kato]
jmathies: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2011-12-20 02:35:03 PST
- Don't need version check for Win2000
- fix possible memory leak
- use bool instead of BOOL as possible
Comment 1 Makoto Kato [:m_kato] 2011-12-20 02:55:51 PST
Created attachment 583106 [details] [diff] [review]
fix
Comment 2 Makoto Kato [:m_kato] 2011-12-20 02:58:31 PST
Created attachment 583107 [details] [diff] [review]
fix v1.1
Comment 3 Jim Mathies [:jimm] 2011-12-20 03:16:30 PST
Comment on attachment 583107 [details] [diff] [review]
fix v1.1

Review of attachment 583107 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/system/win32/nsNotifyAddrListener.cpp
@@ +449,1 @@
>  nsNotifyAddrListener::CheckICSStatus(PWCHAR aAdapterName)

How often is this called? Might be good to move to coinit/couninit calls up so they are only called once for the class.

@@ +458,5 @@
>      HRESULT hr;
>  
>      hr = CoInitializeEx(NULL, COINIT_MULTITHREADED);
>      if (FAILED(hr))
> +        return false;

nits - for added cleanup, why not ditch HRESULT hr, and do

if (FAILED(CoInit())
  return false;

and below here, similarly, there are two |if (SUCCEEDED(hr))| calls, but it looks like the whole block could be wrapped up:

if (SUCCEEDED(CoCreateInstance(...
{
  connectionVariant.punkVal->Release();
  if (SUCCEEDED(connection->GetProperties(..
  {
     ..
  }
}
Comment 4 Makoto Kato [:m_kato] 2011-12-20 23:10:31 PST
(In reply to Jim Mathies [:jimm] from comment #3)
> Comment on attachment 583107 [details] [diff] [review]
> fix v1.1
> 
> Review of attachment 583107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/system/win32/nsNotifyAddrListener.cpp
> @@ +449,1 @@
> >  nsNotifyAddrListener::CheckICSStatus(PWCHAR aAdapterName)
> 
> How often is this called? Might be good to move to coinit/couninit calls up
> so they are only called once for the class.

This implementation is NS_IMPL_THREADSAFE_ISUPPORTS, so caller isn't on specific thread.  We will have to call coinit per call.  But since CheckIsGateway is called from loop, we should move it to CheckAdaptersAddresses.
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-15 09:19:16 PST
https://hg.mozilla.org/mozilla-central/rev/a3f63d363535

Note You need to log in before you can comment on or make changes to this bug.