Closed Bug 657240 Opened 10 years ago Closed 10 years ago

Incorrect use of NS_ProxyRelease in nsAsyncFaviconHelpers can lead to double-frees

Categories

(Toolkit :: Places, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

I think the use of NS_ProxyRelease in AsyncFaviconHelpers.cpp is wrong.

For example:

AsyncAssociateIconToPage::~AsyncAssociateIconToPage()
{
  nsCOMPtr<nsIThread> thread;
  (void)NS_GetMainThread(getter_AddRefs(thread));
  if (mCallback) {
    (void)NS_ProxyRelease(thread, mCallback, PR_TRUE);
  }
  if (mFaviconSvc) {
    (void)NS_ProxyRelease(thread, mFaviconSvc, PR_TRUE);
  }
}

At the end of this destructor, mCallback and mFaviconSvc will be released again when the smart pointers are destroyed.

Contrast this with how NS_ProxyRelease is used in nsDOMWorker::~nsDomWorker:

  nsIPrincipal* principal;
  mPrincipal.forget(&principal);
  if (principal) {
    NS_ProxyRelease(mainThread, principal, PR_FALSE);
  }
I'll fix this, because I'm running into it as I try to write a new patch for bug 655270.
Assignee: nobody → justin.lebar+bug
Blocks: 655270
Ooh, I see.

I was only looking at nsProxyRelease.cpp.  In the .h file, it has sane wrappers which call forget() on smart pointers.

The reason I was seeing crashes in my code was that added an NS_ISUPPORTS_CAST around the smart pointer, which resulted in calling the raw pointer version of NS_ProxyRelease.

False alarm.  Sorry!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Group: core-security
You need to log in before you can comment on or make changes to this bug.