620194 - "ASSERTION: nsLoadGroup not thread-safe" under nsWyciwygCloseEvent::~nsWyciwygCloseEvent

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: stacks

Seen several times during fuzzing on Linux.  I have stacks but no steps to reproduce.

###!!! ASSERTION: nsLoadGroup not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file netwerk/base/src/nsLoadGroup.cpp, line 204

###!!! ASSERTION: nsSimpleURI not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file netwerk/base/src/nsSimpleURI.cpp, line 77

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this happening at shutdown?  I can see this happening if the NS_DispatchToMainThread in ~nsWyciwygAsyncEvent fails: note that all the callstacks show the runnable method being destroyed there under the nsCOMPtr destructor.

Looks like a regression from bug 599127.

Comment 2

[Jesse Ruderman]

Yes, this is happening during shutdown.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sounds like we should leak the channel rather than doing a wrong-thread release, in that situation.  :(  We have to do that a few other places in necko too.  :(

Comment 4

[Michal Novotny [:michal]]

We shutdown the cache IO thread in nsCacheService::Shutdown() which happens on "xpcom-shutdown". It should be possible to post events to the main thread until this point, right? So is there any reason why NS_DispatchToMainThread could fail?


  // ensure we free our ref before event gets released on main thread
  mChannel = 0;
  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

We release the reference that is hold by mChannel, but event references the channel too. So is it still possible that the event is dispatched and processed on the main thread while we hold the reference on the background thread? If yes then this concept with MainReleaseNoOp() can't work reliably and we need to find another mechanism to ensure releasing the WyciwygChannel on the main thread.

The other option is to make nsLoadGroup and nsSimpleURI threadsafe.

Comment 5

[Jesse Ruderman]

> The other option is to make nsLoadGroup and nsSimpleURI threadsafe.

In some of my logs for this bug, there are thread-safety warnings for ~5 additional classes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> So is it still possible that the event is dispatched and processed
> on the main thread while we hold the reference on the background thread?

Oh, hmm.  Yes, it's possible, I suppose; if we post the event and then lose our timeslice and then it runs.

So yes, I think this code is just broken.  Is there a reason you're reinventing NS_ProxyRelease here, btw?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The other option is to make nsLoadGroup and nsSimpleURI threadsafe.

I don't think that's an option, fwiw.  The channel can entrain lots of other non-threadsafe stuff too; we should just proxy the release to the main thread correctly and be done with it.

Comment 8

[Michal Novotny [:michal]]

Attachment: fix

(In reply to comment #6)
> So yes, I think this code is just broken.  Is there a reason you're reinventing
> NS_ProxyRelease here, btw?

There is no reason to not use NS_ProxyRelease. Attached patch fixes it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 499418 [details] [diff] [review]
fix

r=me

Comment 10

[Michal Novotny [:michal]]

Does this change need superreview?

Comment 11

[Boris Zbarsky [:bzbarsky]]

imo, no.

Comment 12

[Dão Gottwald [:dao]]

Needs approval or blocking state to land.

Comment 13

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/840641b4d465