Closed
Bug 632496
Opened 14 years ago
Closed 9 years ago
nsHttpHandler::InPrivateBrowsingMode() breaks on non main thread
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
Assignee: nobody → mcmanus
![]() |
||
Comment 2•14 years ago
|
||
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 | ||
Comment 3•14 years ago
|
||
proxy the init work to the main thread via a runnable event
Attachment #511280 -
Flags: review?(honzab.moz)
![]() |
||
Comment 4•14 years ago
|
||
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-
![]() |
||
Comment 5•14 years ago
|
||
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•14 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•14 years ago
|
||
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)
![]() |
||
Updated•14 years ago
|
Blocks: pipelining-review
![]() |
||
Comment 8•13 years ago
|
||
We could post and wait when initializing PB service not on the main thread. The same approach SSL error reporting does.
![]() |
||
Comment 10•13 years ago
|
||
Comment on attachment 512861 [details] [diff] [review] init on main thread v3 Patrick, please check comment 8 and 9.
![]() |
||
Comment 11•13 years ago
|
||
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•13 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•9 years ago
|
||
no longer exists
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•