729469 - Thread leak in nsUrlClassifierDBService.cpp

Status: REOPENED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Heikki Levanto]

Working on an embedded system, we noticed a thread leak in nsUrlClassifierDBService.cpp near line 4165:

  // Start the background thread.
  rv = NS_NewThread(&gDbBackgroundThread);
  if (NS_FAILED(rv))
    return rv;

  mWorker = new nsUrlClassifierDBServiceWorker();
  if (!mWorker)
    return NS_ERROR_OUT_OF_MEMORY;

  rv = mWorker->Init(gethashNoise, mPrefixSet);
  if (NS_FAILED(rv)) {

    mWorker = nsnull;
    return rv;
  }

If, for some reason, mWorker->Init() fails, mWorker is set to null, but the thread continues to exist. It will never be killed. This happened to us with problems with the profile path.  This code seems to be run every time something is loaded from the net (not from local file), leaking several threads every time a new page is loaded.  


The fix is simple. Add the following after the line mWorker = nsnull; before the return:
  nsIThread *backgroundThread = gDbBackgroundThread;
  gDbBackgroundThread = nsnull;
  backgroundThread->Shutdown();
  NS_RELEASE(backgroundThread);

I am not 100% sure if the fix is correct, it is just copied from a place where the thread is shut down. Looks like there could be another leak in the case the new nsUrlClassifierDBServiceWorker() fails, but I have not seen that in practice, nor do I know what policy (if any) covers out-of-memory situations.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Gian-Carlo, dcamp, can you take a look please?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Analysis and suggested fix look ok to me. Do you already have a patch?

Comment 3

[Josh Matthews [:jdm]]

The safest way to fix this would be to store the new thread in a local nsCOMPtr until all operations are successful:

nsCOMPtr<nsIThread> backgroundThread;
rv = NS_NewThread(getter_AddRefs(gDbBackgroundThread);
...
rv = mWorker->Init(gethashNoise, mPrefixSet);
...
backgroundThread.forget(&gDbBackgroundThread);

Comment 4

[Josh Matthews [:jdm]]

Although I guess that doesn't actually trigger Shutdown, which is pretty important.

Comment 5

[Heikki Levanto]

(In reply to Josh Matthews [:jdm] from comment #4)
> Although I guess that doesn't actually trigger Shutdown, which 
> is pretty important.

It could be argued that if Init() fails, calling ShutDown() should not be necessary, or even desirable. But I don't know the code well enough to say, nor the coding conventions for such.

Comment 6

[François Marier [:francois]]

Henry has overhauled this code recently so this leak may now be fixed.

Comment 7

[Henry Chang [:hchang]]

I didn't make any change to gDbBackgroundThread. Instead, the new thread I introduced
is in Classifier [1].

[1] http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/toolkit/components/url-classifier/Classifier.h#229

Comment 8

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.