Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation)
Categories
(Core :: Networking, defect, P2)
Tracking
()
People
(Reporter: Gijs, Assigned: acreskey, NeedInfo)
References
Details
(Whiteboard: [necko-triaged][necko-priority-queue])
Attachments
(1 file)
The instance that tripped filing bug 1570616 was using CoCreateInstance in the parental controls service. In bug 1584613 this was replaced with a registry read.
This happens on the main thread when initializing the HTTP handler code. The HTTP handler code then has a member that independently tracks whether parental controls are available.
I suspect the mirroring is there because nsHttpHandler needs access off-mainthread. One potential improvement that wouldn't require too many API changes would be to avoid the mirroring, create a threadsafe API on nsParentalControlService and back it with a threadsafe member there which we initialize lazily (that is, send off an idle task to fetch the information off-main-thread when the service is initialized, and fall back to the sync mainthread fetch if we need the information before we've fetched it on the idle task). However, I'm not super familiar with this code and I don't know if the size of the problem warrants that level of complexity.
The call to the parental controls service during the Http handler init was added in bug 1891483 recently.
Dennis, how early do we need that information. Could we defer that operation to a later time?
Note that we also check it in TRRService::Init - which happens relatively soon after startup
|
||
Comment 3•2 months ago
|
||
I didn't add the call in the Http Handler Init in 1891483, that was already there. I only added a read of the local member which was already set. We need this information to know whether to use Encrypted Client Hello (or not) when making TLS connections. We don't want to use it if parental controls are enabled. I expect we need this information very early in startup no matter what (considering the TRR use case as well) - but it sounds like moving it off-main thread is a reasonable way to go..
|
||
Updated•2 months ago
|
|
||
Updated•2 months ago
|
|
|
||
Updated•2 months ago
|
- make mParentalControlsEnabled an atomic variable.
- Dispatch this code block in a runnable to a background thread
nsCOMPtr<nsIParentalControlsService> pc =
do_CreateInstance("@mozilla.org/parental-controls-service;1");
if (pc) {
pc->GetParentalControlsEnabled(&mParentalControlEnabled);
}
- add a method that returns that value
- make the TRR service call into the nsHttpHandler code here
|
Assignee | |
Updated•1 month ago
|
|
|
Assignee | |
Comment 5•12 days ago
|
||
Create a background task to query the state of the parental controls service to prevent main thread blockage.
|
|
Assignee | |
Comment 6•12 days ago
|
||
(In reply to Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }} from comment #4)
- make mParentalControlsEnabled an atomic variable.
- Dispatch this code block in a runnable to a background thread
nsCOMPtr<nsIParentalControlsService> pc = do_CreateInstance("@mozilla.org/parental-controls-service;1"); if (pc) { pc->GetParentalControlsEnabled(&mParentalControlEnabled); }
- add a method that returns that value
- make the TRR service call into the nsHttpHandler code here
Thanks Valentin.
We can discuss in the patch; I've implemented this and while I'm not familiar with the startup order, we may end up racing on the background thread determination of the parental controls state.
Pushed by acreskey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/57a8ef5f87bf Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation) r=necko-reviewers,kershaw
|
||
Comment 8•2 days ago
•
|
||
Backed out for causing multiple failures.
- Push with failures - mochitests failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/satchel/test/test_bug_511615.html | assertion count 1 is more than expected 0 assertions
- Push with failures - another mochitests failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/mochitest/creditCard/test_basic_creditcard_autocomplete_form.html | assertion count 1 is more than expected 0 assertions
- Push with failures - gl failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conf/generated/test_2_conformance__glsl__misc__glsl-vertex-branch.html | assertion count 1 is more than expected 0 assertions
- Push with failures - mochitests plain failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_abort_smooth_scroll_by_instant_scroll.html | assertion count 2 is more than expected 0 assertions
- Push with failures - xpcshell failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/unit/test_activeStatus.js | xpcshell return code: 0
- Push with failures - mda failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | dom/media/webrtc/tests/mochitests/test_peerConnection_bug1227781.html | assertion count 1 is more than expected 0 assertions
- Push with failures - wpt failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | /css/css-content/attr-case-sensitivity-001.html | assertion count 1 is more than expected 0 assertions
- Push with failures - python failures
- Failure Log
- Failure line: testing/mochitest/tests/python/test_mochitest_integration.py::test_output_extra_args[plain-mochitest-args.ini] TEST-UNEXPECTED-FAIL
Description
•