1893948 - Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation)

Reporter

Description

•

10 months ago

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.

Valentin Gosu [:valentin] (he/him)

Comment 1

•

10 months ago

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?

Flags: needinfo?(djackson)

Comment 2

•

10 months ago

Note that we also check it in TRRService::Init - which happens relatively soon after startup

Dennis Jackson

Comment 3

•

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

Flags: needinfo?(djackson)

Randell Jesup [:jesup] (needinfo me)

Updated

•

10 months ago

Severity: -- → S3

Priority: -- → P2

Whiteboard: [necko-triaged][necko-new]

Kershaw Chang [:kershaw]

Updated

•

10 months ago

Whiteboard: [necko-triaged][necko-new] → [necko-triaged][necko-priority-new]

Kershaw Chang [:kershaw]

Updated

•

10 months ago

Whiteboard: [necko-triaged][necko-priority-new] → [necko-triaged][necko-priority-queue]

Valentin Gosu [:valentin] (he/him)

Comment 4

•

9 months ago

make mParentalControlsEnabled an atomic variable.
Dispatch this code block in a runnable to a background thread

https://searchfox.org/mozilla-central/rev/2e7374599f4020d0490ec8c5e877c179c9e153c9/netwerk/protocol/http/nsHttpHandler.cpp#471-475

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

Andrew Creskey [:acreskey]

Assignee

Updated

•

9 months ago

Assignee: nobody → acreskey

Andrew Creskey [:acreskey]

Assignee

Comment 5

•

8 months ago

Attached file Bug 1893948 - Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation) r=#necko-reviewers — Details

Create a background task to query the state of the parental controls service to prevent main thread blockage.

Andrew Creskey [:acreskey]

Assignee

Comment 6

•

8 months 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

https://searchfox.org/mozilla-central/rev/2e7374599f4020d0490ec8c5e877c179c9e153c9/netwerk/protocol/http/nsHttpHandler.cpp#471-475
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.

Pulsebot

Comment 7

•

8 months ago

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

Serban Stanca [:SerbanS]

Comment 8

•

8 months ago

•

Edited

Backed out for causing multiple failures.

Backout link

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

Flags: needinfo?(acreskey)

Pulsebot

Comment 9

•

7 months ago

Pushed by acreskey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f6e4850b977 Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation) r=necko-reviewers,kershaw

Treeherder Bug Filer

Updated

•

7 months ago

Regressions: 1906201

tszentpeteri

Comment 10

•

7 months ago

Backed out for causing xpcshell crashes with [@ nsXPCWrappedJS::AddRef]

[task 2024-07-03T23:03:41.681Z] 23:03:41     INFO -  TEST-START | xpcshell.toml:browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
[task 2024-07-03T23:03:42.322Z] 23:03:42  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.toml:browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js | xpcshell return code: 1
[task 2024-07-03T23:03:42.326Z] 23:03:42     INFO -  TEST-INFO took 642ms
[task 2024-07-03T23:03:42.326Z] 23:03:42     INFO -  >>>>>>>
[task 2024-07-03T23:03:42.326Z] 23:03:42     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2024-07-03T23:03:42.326Z] 23:03:42     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2024-07-03T23:03:42.326Z] 23:03:42     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2024-07-03T23:03:42.327Z] 23:03:42     INFO -  running event loop
[task 2024-07-03T23:03:42.327Z] 23:03:42     INFO -  xpcshell.toml:browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js | Starting setupTest
[task 2024-07-03T23:03:42.327Z] 23:03:42     INFO -  (xpcshell/head.js) | test setupTest pending (2)
[task 2024-07-03T23:03:42.327Z] 23:03:42     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2024-07-03T23:03:42.328Z] 23:03:42     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2024-07-03T23:03:42.328Z] 23:03:42     INFO -  (xpcshell/head.js) | test setupTest finished (2)
[task 2024-07-03T23:03:42.328Z] 23:03:42     INFO -  xpcshell.toml:browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js | Starting test_pkcs11
[task 2024-07-03T23:03:42.328Z] 23:03:42     INFO -  (xpcshell/head.js) | test test_pkcs11 pending (2)
[task 2024-07-03T23:03:42.328Z] 23:03:42     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2024-07-03T23:03:42.329Z] 23:03:42     INFO -  "Extension attached"
[task 2024-07-03T23:03:42.329Z] 23:03:42     INFO -  <<<<<<<
[task 2024-07-03T23:04:25.371Z] 23:04:25  WARNING -  PROCESS-CRASH | MOZ_RELEASE_ASSERT(NS_IsMainThread()) (nsXPCWrappedJS::AddRef called off main thread) [@ nsXPCWrappedJS::AddRef] | xpcshell.toml:browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
[task 2024-07-03T23:04:25.375Z] 23:04:25     INFO -  Process type: main
[task 2024-07-03T23:04:25.375Z] 23:04:25     INFO -  Process pid: 3668
[task 2024-07-03T23:04:25.375Z] 23:04:25     INFO -  Mozilla crash reason: MOZ_RELEASE_ASSERT(NS_IsMainThread()) (nsXPCWrappedJS::AddRef called off main thread)
[task 2024-07-03T23:04:25.375Z] 23:04:25     INFO -  Crash dump filename: C:\Users\task_172004359714675\AppData\Local\Temp\xpc-other-b5s3vk1v\9f7c2953-2c79-45e2-a9f6-e9f352da89b1.dmp
[task 2024-07-03T23:04:25.376Z] 23:04:25     INFO -  Operating system: Windows NT
[task 2024-07-03T23:04:25.376Z] 23:04:25     INFO -                    10.0.22621
[task 2024-07-03T23:04:25.376Z] 23:04:25     INFO -  CPU: x86
[task 2024-07-03T23:04:25.376Z] 23:04:25     INFO -       GenuineIntel family 6 model 106 stepping 6
[task 2024-07-03T23:04:25.376Z] 23:04:25     INFO -       8 CPUs
[task 2024-07-03T23:04:25.377Z] 23:04:25     INFO -  Crash reason:  EXCEPTION_BREAKPOINT
[task 2024-07-03T23:04:25.377Z] 23:04:25     INFO -  Crash address: 0x6a9e44f6
[task 2024-07-03T23:04:25.377Z] 23:04:25     INFO -  Process uptime: 1 seconds
[task 2024-07-03T23:04:25.377Z] 23:04:25     INFO -  Thread 26 BackgroundThreadPool #2 (crashed)
[task 2024-07-03T23:04:25.377Z] 23:04:25     INFO -   0  xul.dll!nsXPCWrappedJS::AddRef() [XPCWrappedJS.cpp:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 253 + 0x0]
[task 2024-07-03T23:04:25.378Z] 23:04:25     INFO -       eip = 0x6a9e44f6    esp = 0x1313f160    ebp = 0x1313f168    ebx = 0x1313f1d8
[task 2024-07-03T23:04:25.378Z] 23:04:25     INFO -       esi = 0x137421c0    edi = 0x117c1a80    eax = 0x71cfbe78    ecx = 0x04ab4550
[task 2024-07-03T23:04:25.378Z] 23:04:25     INFO -       edx = 0x00004000 eflags = 0x00000246
[task 2024-07-03T23:04:25.378Z] 23:04:25     INFO -      Found by: given as instruction pointer in context
[task 2024-07-03T23:04:25.378Z] 23:04:25     INFO -   1  xul.dll!nsXPTCStubBase::AddRef() [xptcall.cpp:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 24 + 0x17]
[task 2024-07-03T23:04:25.379Z] 23:04:25     INFO -       eip = 0x6a322fc8    esp = 0x1313f170    ebp = 0x1313f178    ebx = 0x1313f1d8
[task 2024-07-03T23:04:25.379Z] 23:04:25     INFO -       esi = 0x137421c0    edi = 0x117c1a80
[task 2024-07-03T23:04:25.380Z] 23:04:25     INFO -      Found by: call frame info
[task 2024-07-03T23:04:25.380Z] 23:04:25     INFO -   2  xul.dll!mozilla::RefPtrTraits<nsIFactory>::AddRef(nsIFactory*) [RefPtr.h:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 48]
[task 2024-07-03T23:04:25.380Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.380Z] 23:04:25     INFO -   3  xul.dll!nsCOMPtr<nsIFactory>::nsCOMPtr(nsCOMPtr<nsIFactory> const&) [nsCOMPtr.h:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 387]
[task 2024-07-03T23:04:25.381Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.381Z] 23:04:25     INFO -   4  xul.dll!nsFactoryEntry::GetFactory() [nsComponentManager.cpp:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 1371]
[task 2024-07-03T23:04:25.381Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.381Z] 23:04:25     INFO -   5  xul.dll!(anonymous namespace)::EntryWrapper::GetFactory::Matcher::operator()(nsFactoryEntry*) [nsComponentManager.cpp:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 175]
[task 2024-07-03T23:04:25.382Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.382Z] 23:04:25     INFO -   6  xul.dll!mozilla::detail::VariantImplementation<bool,0,nsFactoryEntry *,const mozilla::xpcom::StaticModule *>::match((anonymous namespace)::EntryWrapper::GetFactory::Matcher&&, mozilla::Variant<nsFactoryEntry *,const mozilla::xpcom::StaticModule *>&) [Variant.h:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 266]
[task 2024-07-03T23:04:25.382Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.405Z] 23:04:25     INFO -  36  mozglue.dll!mozilla::interceptor::FuncHook<mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyShared>,void (*)(int, void *, void *) __attribute__((fastcall))>::operator()(int&, void*&, void*&) const [nsWindowsDllInterceptor.h:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 150]
[task 2024-07-03T23:04:25.406Z] 23:04:25     INFO -      Found by: inlining
[task 2024-07-03T23:04:25.406Z] 23:04:25     INFO -  37  mozglue.dll!patched_BaseThreadInitThunk(int, void*, void*) [WindowsDllBlocklist.cpp:7f6e4850b977fc6bd4ea06cfae8b581b8864c3e9 : 562 + 0x13]
[task 2024-07-03T23:04:25.406Z] 23:04:25     INFO -       eip = 0x71cb8290    esp = 0x1313f894    ebp = 0x1313f8c0    ebx = 0x76f259a0
[task 2024-07-03T23:04:25.406Z] 23:04:25     INFO -       esi = 0x71e29020    edi = 0x04a8c518
[task 2024-07-03T23:04:25.407Z] 23:04:25     INFO -      Found by: call frame info
[task 2024-07-03T23:04:25.407Z] 23:04:25     INFO -  38  ntdll.dll!__RtlUserThreadStart + 0x2a
[task 2024-07-03T23:04:25.407Z] 23:04:25     INFO -       eip = 0x77bcc10b    esp = 0x1313f8c8    ebp = 0x1313f918    ebx = 0x04a8c518
[task 2024-07-03T23:04:25.407Z] 23:04:25     INFO -       esi = 0x768e7b90    edi = 0x76f259a0
[task 2024-07-03T23:04:25.408Z] 23:04:25     INFO -      Found by: call frame info
[task 2024-07-03T23:04:25.408Z] 23:04:25     INFO -  39  ntdll.dll!_RtlUserThreadStart + 0x1a
[task 2024-07-03T23:04:25.408Z] 23:04:25     INFO -       eip = 0x77bcc08f    esp = 0x1313f920    ebp = 0x1313f928    ebx = 0x04a8c518
[task 2024-07-03T23:04:25.408Z] 23:04:25     INFO -       esi = 0x768e7b90    edi = 0x76f259a0
[task 2024-07-03T23:04:25.408Z] 23:04:25     INFO -      Found by: call frame info

Pulsebot

Comment 11

•

7 months ago

Pushed by acreskey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e1eae15c143 Avoid sync IO in parental controls service (provide threadsafe + lazily-looked-up implementation) r=necko-reviewers,kershaw

Sandor Molnar[:smolnar]

Comment 12

•

7 months ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/8e1eae15c143

Status: NEW → RESOLVED

Closed: 7 months ago

status-firefox130: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 130 Branch

Andrew Creskey [:acreskey]

Assignee

Updated

•

7 months ago

Regressions: 1908228

Andrew Creskey [:acreskey]

Assignee

Updated

•

7 months ago

Flags: needinfo?(acreskey)