nsHttpHandler::InPrivateBrowsingMode() breaks on non main thread

RESOLVED WONTFIX

Status

()

Core
Networking: HTTP
RESOLVED WONTFIX
7 years ago
2 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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
(Assignee)

Updated

7 years ago
Assignee: nobody → mcmanus
(Assignee)

Comment 1

7 years ago
Created attachment 510704 [details] [diff] [review]
init on main thread v1
Attachment #510704 - Flags: review?(honzab.moz)
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.
Attachment #510704 - Flags: review?(honzab.moz) → review-
(Assignee)

Updated

7 years ago
Blocks: 603508
(Assignee)

Comment 3

7 years ago
Created attachment 511280 [details] [diff] [review]
init on main thread via runnable event v2

proxy the init work to the main thread via a runnable event
Attachment #511280 - Flags: review?(honzab.moz)
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
Attachment #511280 - Flags: review?(honzab.moz) → review-
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

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
Created attachment 512861 [details] [diff] [review]
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.
Attachment #510704 - Attachment is obsolete: true
Attachment #511280 - Attachment is obsolete: true
Attachment #512861 - Flags: review?(honzab.moz)
(Assignee)

Updated

7 years ago
Blocks: 603503
Blocks: 659760
We could post and wait when initializing PB service not on the main thread.  The same approach SSL error reporting does.

Comment 9

6 years ago
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.
Attachment #512861 - Flags: review?(honzab.moz)
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

2 years ago
no longer exists
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.