1553855 - Classifier Update thread should be a LazyIdle thread

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

The Classifier Update thread is rarely active, but is created on startup and lives until shutdown. It should be moved to a LazyIdle thread to avoid memory use for the 99.9% of the time it's not active.

The extra memory usage is more important on low-memory desktops and on Android devices.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1553855: Make the Classifier Update thread a LazyIdle thread r=dimi

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bb757ffb7e9
Make the Classifier Update thread a LazyIdle thread r=dimi

Comment 3

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 5bb757ffb7e9 (Bug 1553855) for Classifier.cpp assertion failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5bb757ffb7e9f9c89cd327e06a6888548362aba4&selectedJob=248652719

Backout link: https://hg.mozilla.org/integration/autoland/rev/795082d54c998ff568d64472e8f0af828a011eae

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248652719&repo=autoland&lineNumber=1560

[task 2019-05-28T01:36:55.143Z] 01:36:55 INFO - [1049, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2019-05-28T01:36:55.718Z] 01:36:55 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: Failed to get base domain!: file /builds/worker/workspace/build/src/ipc/glue/BackgroundUtils.cpp, line 360
[task 2019-05-28T01:36:56.260Z] 01:36:56 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0x7f20a3425410 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 108
[task 2019-05-28T01:36:56.481Z] 01:36:56 INFO - GECKO(1074) | [Parent 1074, StreamTrans #3] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
[task 2019-05-28T01:36:56.498Z] 01:36:56 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 994
[task 2019-05-28T01:36:56.516Z] 01:36:56 INFO - [1049, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2019-05-28T01:36:56.717Z] 01:36:56 INFO - GECKO(1074) | [Parent 1074, StreamTrans #4] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
[task 2019-05-28T01:36:56.721Z] 01:36:56 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 994
[task 2019-05-28T01:36:56.782Z] 01:36:56 INFO - [1049, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2019-05-28T01:36:57.729Z] 01:36:57 INFO - GECKO(1074) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpbFoHwO.mozrunner/runtests_leaks_tab_pid1208.log
[task 2019-05-28T01:36:57.936Z] 01:36:57 INFO - GECKO(1074) | Assertion failure: NS_GetCurrentThread() == self->mUpdateThread (MUST be on update thread), at /builds/worker/workspace/build/src/toolkit/components/url-classifier/Classifier.cpp:690
[task 2019-05-28T01:37:36.198Z] 01:37:36 INFO - GECKO(1074) | #01: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1165]
[task 2019-05-28T01:37:36.198Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO - GECKO(1074) | #02: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:486]
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO - GECKO(1074) | #03: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:304]
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO - GECKO(1074) | #04: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO - GECKO(1074) | #05: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO - GECKO(1074) | #06: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:457]
[task 2019-05-28T01:37:36.199Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.362Z] 01:37:36 INFO - GECKO(1074) | #07: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:204]
[task 2019-05-28T01:37:36.362Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO - GECKO(1074) | #08: libpthread.so.0 + 0x76ba
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO - GECKO(1074) | #09: libc.so.6 + 0x10741d
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO -
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO - GECKO(1074) | #10: ??? (???:???)
[task 2019-05-28T01:37:36.363Z] 01:37:36 INFO - GECKO(1074) | ExceptionHandler::GenerateDump cloned child 1221
[task 2019-05-28T01:37:36.364Z] 01:37:36 INFO - GECKO(1074) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2019-05-28T01:37:36.366Z] 01:37:36 INFO - GECKO(1074) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2019-05-28T01:37:36.367Z] 01:37:36 INFO - GECKO(1074) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2019-05-28T01:37:36.368Z] 01:37:36 INFO - GECKO(1074) | [Child 1208, Chrome_ChildThread] WARNING: pipe error (42): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 358
[task 2019-05-28T01:37:36.369Z] 01:37:36 INFO - GECKO(1074) | [Child 1208, Chrome_ChildThread] WARNING: pipe error (41): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 358
[task 2019-05-28T01:37:36.370Z] 01:37:36 INFO - GECKO(1074) | [Child 1208, Chrome_ChildThread] WARNING: pipe error (3): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 358
[task 2019-05-28T01:37:36.371Z] 01:37:36 INFO - GECKO(1074) | Exiting due to channel error.
[task 2019-05-28T01:37:36.372Z] 01:37:36 INFO - GECKO(1074) | [GFX1-]: Receive IPC close with reason=AbnormalShutdown
[task 2019-05-28T01:37:36.372Z] 01:37:36 INFO - GECKO(1074) | [Child 1155, Chrome_ChildThread] WARNING: pipe error (3): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 358
[task 2019-05-28T01:37:36.373Z] 01:37:36 INFO - GECKO(1074) | Exiting due to channel error.
[task 2019-05-28T01:37:36.374Z] 01:37:36 INFO - GECKO(1074) | [GFX1-]: Receive IPC close with reason=AbnormalShutdown
[task 2019-05-28T01:37:36.374Z] 01:37:36 INFO - GECKO(1074) | [Child 1134, Chrome_ChildThread] WARNING: pipe error (3): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 358
[task 2019-05-28T01:37:36.375Z] 01:37:36 INFO - GECKO(1074) | Exiting due to channel error.
[task 2019-05-28T01:39:47.189Z] 01:39:47 INFO - runtests.py | Waiting for browser...
[task 2019-05-28T01:39:47.190Z] 01:39:47 INFO - TEST-INFO | Main app process: exit 11
[task 2019-05-28T01:39:47.191Z] 01:39:47 ERROR - TEST-UNEXPECTED-FAIL | ShutdownLeaks | process() called before end of test suite
[task 2019-05-28T01:39:47.192Z] 01:39:47 INFO - Buffered messages finished
[task 2019-05-28T01:39:47.193Z] 01:39:47 ERROR - TEST-UNEXPECTED-FAIL | automation.py | application terminated with exit code 11
[task 2019-05-28T01:39:47.194Z] 01:39:47 INFO - runtests.py | Application ran for: 0:03:00.109465

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

Another failure introduced by these changes: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248652852&repo=autoland&lineNumber=1376

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1553855: Handle on-thread checks in Classifier more generically r=dimi

Depends on D32529

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

This makes the on-thread checks work with any nsIThread-type; this should work with a SharedThreadPool in the future if we move it to that.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=248692000&revision=b144c086ead5688be980755e8266e0a9c774d6b9

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Also (android): https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=248692000&revision=fbd32e5b568e1b0e9435ed4a6dbc6e14ff4b1af7

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46cfc6e018ba
Make the Classifier Update thread a LazyIdle thread r=dimi

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out for assertion failures on LazyIdleThread.cpp

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff89e5a0dc662ab150000d427da4b744e6575064

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=46cfc6e018bada56fff63c6ab6a000b31ba86437&selectedJob=249115930

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249115930&repo=mozilla-inbound&lineNumber=26226

05:42:50 INFO - TEST-START | UrlClassifierFailUpdate.CheckTableReset
05:42:50 INFO - Assertion failure: mOwningEventTarget->IsOnCurrentThread(), at z:/build/build/src/xpcom/threads/LazyIdleThread.cpp:364
05:43:09 INFO - #01: mozilla::safebrowsing::Classifier::AsyncApplyUpdates(nsTArray<RefPtr<mozilla::safebrowsing::TableUpdate> > const &,std::function<void > const &) [toolkit/components/url-classifier/Classifier.cpp:730]
05:43:09 INFO - #02: nsresult mozilla::detail::RunnableFunction<`lambda at z:/build/build/src/toolkit/components/url-classifier/tests/gtest/Common.cpp:51:72'>::Run() [xpcom/threads/nsThreadUtils.h:562]
05:43:09 INFO - #03: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1165]
05:43:09 INFO - #04: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:486]
05:43:09 INFO - #05: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:303]
05:43:09 INFO - #06: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:309]
05:43:09 INFO - #07: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
05:43:09 INFO - #08: nsThread::ThreadFunc(void *) [xpcom/threads/nsThread.cpp:457]
05:43:10 INFO - #09: PR_NativeRunThread [nsprpub/pr/src/threads/combined/pruthr.c:406]
05:43:10 INFO - #10: static unsigned int pr_root(void *) [nsprpub/pr/src/md/windows/w95thred.c:138]
05:43:10 INFO - #11: ucrtbase.dll + 0x1c4be
05:43:10 INFO - #12: KERNEL32.DLL + 0x13034
05:43:10 INFO - #13: static void patched_BaseThreadInitThunk(int, void *, void *) [mozglue/build/WindowsDllBlocklist.cpp:701]
05:43:10 INFO - #14: ntdll.dll + 0x71461
05:43:10 INFO - mozcrash INFO | Copy/paste: Z:\task_1559193970\build\win32-minidump_stackwalk.exe Z:\task_1559193970\build\tests\gtest\347314e5-af4d-4ded-8472-a88e9255ad22.dmp Z:\task_1559193970\build\symbols
05:43:21 INFO - mozcrash INFO | Saved minidump as Z:\task_1559193970\build\blobber_upload_dir\347314e5-af4d-4ded-8472-a88e9255ad22.dmp
05:43:21 INFO - mozcrash INFO | Saved app info as Z:\task_1559193970\build\blobber_upload_dir\347314e5-af4d-4ded-8472-a88e9255ad22.extra
05:43:21 WARNING - PROCESS-CRASH | gtest | application crashed [@ mozilla::LazyIdleThread::Dispatch(already_AddRefed<nsIRunnable>,unsigned int)]

Other failure examples:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249117083&repo=mozilla-inbound&lineNumber=39773
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249115929&repo=mozilla-inbound&lineNumber=4784

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jesup, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Dimi Lee [:dimi]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #11)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jesup, could you have a look please?
For more information, please visit auto_nag documentation.

I'll probably take a look at this next week.

Comment 13

[Dimi Lee [:dimi]]

I am fixing the testcase errors after introducing LazyIdleThread. Steal this bug from :jesup

Comment 14

[Dimi Lee [:dimi]]

Attachment: Bug 1553855 - P1. Make the Classifier Update thread a LazyIdle thread. r?gcp

Safe Browsing update thread wakes up every 30 mins to update tables from google
and 60 mins to update tables from mozilla.

Since the update thread doesn't have always to be alive, we change the
update thread to be a LazyIdle thread instead.

Comment 15

[Dimi Lee [:dimi]]

Attachment: Bug 1553855 - P2. Fix Safe Browsing testcase errors after introducing LazyIdle thread. r?gcp

A LazyIdle thread should be created and removed by the same thread. This
patch fixes testcases that trigger the assertion.

Depends on D49874

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f506810a0a9
P1. Make the Classifier Update thread a LazyIdle thread. r=gcp
https://hg.mozilla.org/integration/autoland/rev/c39004cf92c4
P2. Fix Safe Browsing testcase errors after introducing LazyIdle thread. r=gcp

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0f506810a0a9
https://hg.mozilla.org/mozilla-central/rev/c39004cf92c4

Comment 18

[Chris Peterson [:cpeterson]]

firefox71=wontfix because I assume we don't need to uplift this change to GeckoView Beta (71).