1817463 - UrlClassifier updates should stop when the user has been idle for a while

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Florian Quèze [:florian]]

Here is a profile I captured when leaving Firefox running all night long showing only a single about:blank tab: https://share.firefox.dev/3I9ge58

Every 1800s (30min), there's a network request to https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$ct=application/x-protobuf&key=... and a new "Classifier Update" thread that takes about 1.4s of CPU time (causing significant power use).

This seems wasteful when the user is completely away from the computer.

I see the timer duration is set by the remote server, with the code at https://searchfox.org/mozilla-central/rev/9de332d5c8faac58dc1232b8a6383ce6cb1400f4/toolkit/components/url-classifier/UrlClassifierListManager.jsm#324-327 ensuring it's within a [5min;1day] range.

Could we stop this timer when the user is idle, and restart it when the user comes back?

Is there room for optimization in what the Classifier Update threads do? https://share.firefox.dev/416yVPg

Comment 1

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #0)

Is there room for optimization in what the Classifier Update threads do? https://share.firefox.dev/416yVPg

Here is another profile with the FileIO markers: https://share.firefox.dev/3lJ9uTO

At the beginning we touch many files, I think that's when copying the entire directory (mozilla::safebrowsing::Classifier::CopyDirectoryInterruptible).

Then we read C:\Users\Florian\AppData\Local\Mozilla\Firefox\Profiles\hvnrprg7.default-nightly\safebrowsing-updating\google4\goog-phish-proto.vlpset with many read calls, so maybe we are using a smaller buffer than we could?
Then there's 1.2s of CPU without disk I/O.
Then many (small?) write calls to C:\Users\Florian\AppData\Local\Mozilla\Firefox\Profiles\hvnrprg7.default-nightly\safebrowsing-updating\google4\goog-phish-proto-1.vlpset, then a very slow fsync call, taking 43ms. I think when doing an fsync, we fsync the entire file system, so it's likely that we also sync to disk all the files that we copied when we copied the entire directory at the beginning.

And finally it seems we do something to other smaller files, I see 3 more fsync calls:

(PoisonIOInterposer) fsync — C:\Users\Florian\AppData\Local\Mozilla\Firefox\Profiles\hvnrprg7.default-nightly\safebrowsing-updating\google4\goog-malware-proto-1.vlpset
(PoisonIOInterposer) fsync — C:\Users\Florian\AppData\Local\Mozilla\Firefox\Profiles\hvnrprg7.default-nightly\safebrowsing-updating\google4\goog-unwanted-proto-1.vlpset
(PoisonIOInterposer) fsync — C:\Users\Florian\AppData\Local\Mozilla\Firefox\Profiles\hvnrprg7.default-nightly\safebrowsing-updating\google4\goog-badbinurl-proto-1.vlpset

Comment 2

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #0)

Could we stop this timer when the user is idle, and restart it when the user comes back?

Maybe when an update has finished, we could check how long the user has been idle, and if the user has been idle for longer than the timer we are about to set, instead of setting the timer, we could add an observer for when the user comes back (could be a "user-interaction-active" nsObserverService notification), and handle the user returning similarly to how we would handle checking that the list is up to date during startup.

Comment 3

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

The severity field is not set for this bug.
:dimi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Florian Quèze [:florian]]

I attached a patch doing something very similar for the new tab page updates in bug 1821490, that might help as a source of inspiration for this bug.

Comment 5

[Tim Huang[:timhuang] [PTO 1/31 ~ 2/9]]

Attachment: Bug 1817463 - Deferring SafeBrowsing updates if the browser is in idle mode. r?dimi!

This patch implements the behavior for deferring SafeBrowsing updates
when the browser is in idle mode. The update will be deferred until the
next user interaction active.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80b29d7af431
Deferring SafeBrowsing updates if the browser is in idle mode. r=dimi

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae17e25d0f24
Backed out changeset 80b29d7af431 for failures in toolkit/components/url-classifier/tests.

Comment 8

[Serban Stanca [:SerbanS]]

Backed out for for failures in toolkit/components/url-classifier/tests.

• Backout link

---

• Pushes with failures - mochitests failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref.html | Test timed out. -

---

• Pushes with failures - mochitests failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_staticPartition_network.js | Found the expected number of items in the cache - Got +0, expected 1

---

• Pushes with failures - Xpcshell failures
• Failure Log
• Failure line: TEST-UNEXPECTED-TIMEOUT | toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js | Test timed out

Comment 9

[Serban Stanca [:SerbanS]]

Also for these mochitests.

Comment 10

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/ae17e25d0f24

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab5f1ac91158
Deferring SafeBrowsing updates if the browser is in idle mode. r=dimi

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/ab5f1ac91158