338677 - nsUrlClassifierDBService leaks memory

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

mConnection and gEventQ are regular pointers so they need manual release.

We also don't release the reference to the thread queue from mWorker when we're done with it.

Comment 1

[Tony Chang (Google)]

Attachment: v1: patch for branch

This is for branch; I'm working on a patch for trunk.

Comment 2

[Tony Chang (Google)]

Attachment: v1: patch for trunk

Comment 3

[Darin Fisher]

Comment on attachment 222756 [details] [diff] [review]
v1: patch for trunk

>Index: nsUrlClassifierDBService.cpp

> nsUrlClassifierDBServiceWorker::CloseDb()
> {
>   if (mConnection != nsnull) {
>+    NS_RELEASE(mConnection);
>     delete mConnection;
>     mConnection = nsnull;

So, NS_RELEASE(x) will null out |x| as a side-effect.  That means that
|delete x| and |x = nsnull| will both be no-ops, so they should be 
removed.  Besides, I don't think you meant to delete this object! :)


>   gDbBackgroundThread->Shutdown();
>   NS_RELEASE(gDbBackgroundThread);
>+  gDbBackgroundThread = nsnull;

No need for explicit nulling here.


r=darin with those nits picked

Comment 4

[Tony Chang (Google)]

Attachment: v2: patch for branch

remove extra delete and null

Comment 5

[Tony Chang (Google)]

Attachment: v2: patch for trunk

extra null assignment removed

Comment 6

[Darin Fisher]

fixed-on-trunk, fixed1.8.1

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I think the trunk patch doesn't do the right thing:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp&mark=460,461&rev=1.17#460