nsHttpHandler::InPrivateBrowsingMode() tries to lazily init itself (as well as observing changes in private browsing mode), but accessing itself on the network thread (a very normal thing for nsHttpHandler callers to do) generates ASSERTION: nsPrivateBrowsingServiceWrapper not thread-safe gah. Patch it up by initializing itself in Init() from the main thread and then listen for changes

Comment on attachment 510704 [details] [diff] [review] init on main thread v1 We would regress bug 614229 with this. Need to think about this. Probably create and access the service using a proxy to the main thread.

proxy the init work to the main thread via a runnable event

Comment on attachment 511280 [details] [diff] [review] init on main thread via runnable event v2 This is still not good: - we may get to a situation that the state of PB mode is not known on the socket thread when we need it because the RunnableInit has not fired on the main thread so far - according to [1] the service may become available later after call to Init happened, with this patch we would not detect PB mode later in that case at all [1] https://bugzilla.mozilla.org/show_bug.cgi?id=614229#c6

I would rather tend to turn nsPrivateBrowsingServiceWrapper to be threadsafe or fix this in a more general field. Adding some people that would know more.

(In reply to comment #5) > I would rather tend to turn nsPrivateBrowsingServiceWrapper to be threadsafe or > fix this in a more general field. This is tricky, as some of the operations on that class (such as setting privateBrowsingEnabled) are inherently main-thread-only.

This version 1] leaves nshttphandler::inprivatebrowsingmode() unchanged other than adding an assert() to ensure it is only run on the main thread. it still synchronously calls the privatebrowsing service if the local variable is UNKNOWN. 2] as with version 2, out of init(), dispatch to the main thread a routine to set the local variable to ON/OFF if the privatebrowsing service is available at a fairly early time. 3] add a privatebrowsingmode() accessor that returns the tristate (on/off/unknown) of the local variable safe to call from any thread. This satisfies my requirement - the caller can cope conservatively with UNKNOWN.

We could post and wait when initializing PB service not on the main thread. The same approach SSL error reporting does.

That sounds like a good idea to me.

Comment on attachment 512861 [details] [diff] [review] init on main thread v3 Patrick, please check comment 8 and 9.

Comment on attachment 512861 [details] [diff] [review] init on main thread v3 As said before: We could post and wait when initializing PB service not on the main thread. The same approach SSL error reporting does. Patrick, can you please update the patch? I could implement that idea instead if not.

(In reply to Honza Bambas (:mayhemer) from comment #11) > Comment on attachment 512861 [details] [diff] [review] > init on main thread v3 > > As said before: We could post and wait when initializing PB service not on > the main thread. The same approach SSL error reporting does. Patrick, can > you please update the patch? I could implement that idea instead if not. if you want to update it that is fine with me, otherwise I will get to it eventually. This is part of the persistetnt-pipelinine-data patches which are in my second tier of priorities, so I'm not looking a it right now.

no longer exists