632496 - nsHttpHandler::InPrivateBrowsingMode() breaks on non main thread

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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 1

[Patrick McManus [:mcmanus]]

Attachment: init on main thread v1

Comment 2

[Honza Bambas (:mayhemer)]

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.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: init on main thread via runnable event v2

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

Comment 4

[Honza Bambas (:mayhemer)]

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

Comment 5

[Honza Bambas (:mayhemer)]

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.

Comment 6

[(no longer active)]

(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.

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: init on main thread v3

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.

Comment 8

[Honza Bambas (:mayhemer)]

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

Comment 9

[(no longer active)]

That sounds like a good idea to me.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 512861 [details] [diff] [review]
init on main thread v3

Patrick, please check comment 8 and 9.

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Patrick McManus [:mcmanus]]

(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.

Comment 13

[Patrick McManus [:mcmanus]]

no longer exists