350873 - nsStreamLoader code accessed on secondary thread [ASSERTION: nsStreamLoader not thread-safe]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P3)

Description

[Kai Engert (:KaiE:)]

We occassionaly get a console warning about nsStreamLoader not being thread safe.

We identified the cause to be the destructor of nsIStreamLoaderObserver.

Christian recommended to try NS_ProxyRelease.
While this makes the warning go away, it can introduce a deadlock on application exit, because we need to do synchrounous shutdown of all NSS resources, which requires destroying all our objects first.

The deadlock is caused by the main thread dispatching shutdown events, and the PSM modules's shutdown code trying to proxy a destructor call to the main thread.

Comment 1

[Jesse Ruderman]

Loading http://grml.org/ triggers:

###!!! ASSERTION: nsStreamLoader not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/jruderman/trunk/mozilla/netwerk/base/src/nsStreamLoader.cpp, line 65

Comment 2

[Robert Sayre]

Is this really only on the branch? Shouldn't block 1.9 if so.

Comment 3

[Kai Engert (:KaiE:)]

I guess this affects trunk, too, as I'm not aware of relevant changes.

Comment 5

[Carsten Book [:Tomcat]]

(In reply to comment #3)
> I guess this affects trunk, too, as I'm not aware of relevant changes.
> 

confirmed on trunk, my steps to reproduce on trunk are :
->Type gmail.google.com into the url bar
-> Before the Gmail UI
(https://www.google.com/accounts/ServiceLogin?service=mail&passive=true ... )is
loaded completely the Assertion occur

Comment 6

[Mats Palmgren (inactive)]

Loading https://www.bankofamerica.com/ trigger the assertion. (FF Linux)

Comment 7

[Dave Camp (:dcamp)]

(In reply to comment #0)

> Christian recommended to try NS_ProxyRelease.
> While this makes the warning go away, it can introduce a deadlock on
> application exit, because we need to do synchrounous shutdown of all NSS
> resources, which requires destroying all our objects first.
> 
> The deadlock is caused by the main thread dispatching shutdown events, and the
> PSM modules's shutdown code trying to proxy a destructor call to the main
> thread.

Would this actually deadlock?  NS_ProxyRelease proxies the event asynchronously, it wouldn't block the rest of the PSM shutdown.

Comment 8

[Dave Camp (:dcamp)]

Attachment: proxy mListener's release

... so unless I'm wrong, this should work fine.

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 303166 [details] [diff] [review]
proxy mListener's release

... and apparently I am, because I just ran into a shutdown deadlock.

Comment 10

[Dave Camp (:dcamp)]

Attachment: v2

so actually the hang I was seeing was caused by bug 417404.  New patch cleans things up a bit.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 303208 [details] [diff] [review]
v2

Using this patch (only), I crash when visiting some SSL sites.
Using this patch combined with the patch from bug 417404, it works and the assertion is gone.

This tells me, we must really wait for 417404 before we can check this in.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 303208 [details] [diff] [review]
v2

sr=dveditz with the caveat about landing 417404 too.

Comment 13

[Dave Camp (:dcamp)]

Checking in nsNSSCallbacks.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp,v  <--  nsNSSCallbacks.cpp
new revision: 1.62; previous revision: 1.61
done