712243 - cleanup nsNotifyAddrListener

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Makoto Kato [:m_kato]]

- Don't need version check for Win2000
- fix possible memory leak
- use bool instead of BOOL as possible

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 2

[Makoto Kato [:m_kato]]

Attachment: fix v1.1

Comment 3

[Jim Mathies [:jimm]]

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]]

(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 5

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f63d363535

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a3f63d363535