cleanup nsNotifyAddrListener

RESOLVED FIXED in mozilla13

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla13
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
- Don't need version check for Win2000
- fix possible memory leak
- use bool instead of BOOL as possible
(Assignee)

Updated

6 years ago
Assignee: nobody → m_kato
(Assignee)

Comment 1

6 years ago
Created attachment 583106 [details] [diff] [review]
fix
Attachment #583106 - Flags: review?(jmathies)
(Assignee)

Comment 2

6 years ago
Created attachment 583107 [details] [diff] [review]
fix v1.1
Attachment #583106 - Attachment is obsolete: true
Attachment #583106 - Flags: review?(jmathies)
Attachment #583107 - Flags: review?(jmathies)

Comment 3

6 years ago
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(..
  {
     ..
  }
}
Attachment #583107 - Flags: review?(jmathies) → review+
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f63d363535
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/a3f63d363535
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.