When I discuss with a mobile network operator about reducing power usage on Firefox Android, they point out the following. For safe browsing, Firefox seems to access google server etc to get safe browsing black list. This connection keeps 3 or 4 minutes, and send keep-alive packet during keeping connection per 1 minute. When disconnecting it after 3 or 4 minutes after getting safe browsing list, FIN/RST is sent. Due to sending keep-alive packet and FIN/RST packet, modem wakes up from deep sleep state, so power usage is increment by keep-alive connection. They calculate that Firefox reduces 5% down of power usage for modem if not using keep-alive for safe browsing servers.

I don't think any of our connections to that server involve multiple messages where the full body isn't know in advance, so we might be able to just disable keep-alive for it entirely.

Untested, but this looks like it might be the answer.

Oh hey, more code.

Now with correct import.

Comment on attachment 8491147 [details] [diff] [review] Disable safebrowsing keepalive for Fennec. v1 Review of attachment 8491147 [details] [diff] [review]: ----------------------------------------------------------------- Blame says dcamp gets the first review request; please redirect if appropriate!

Comment on attachment 8491168 [details] [diff] [review] Disable safebrowsing keepalive for Fennec (C++ part). v3 Review of attachment 8491168 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp @@ +6,5 @@ > #include "nsCRT.h" > #include "nsIHttpChannel.h" > #include "nsIObserverService.h" > +#include "nsIPrefBranch.h" > +#include "nsIPrefService.h" Do you need those given that you just use a direct mozilla::Preferences::GetXXX query?

Monica, do you agree with my comment 1? In that case we might want to do this everywhere.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8) > Do you need those given that you just use a direct > mozilla::Preferences::GetXXX query? Probably not. This is cargo-culted from nsUrlClassifierDBService.cpp.

Comment on attachment 8491147 [details] [diff] [review] Disable safebrowsing keepalive for Fennec. v1 Review of attachment 8491147 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js @@ +223,5 @@ > let channel = Services.io.newChannelFromURI(uri); > channel.loadFlags = loadFlags; > > + // Disable keepalive if specified. > + if (Services.prefs.getBoolPref("urlclassifier.disable_keepalive")) { This needs a try-catch. nsIPrefBranch always makes me sad.

Comment on attachment 8491147 [details] [diff] [review] Disable safebrowsing keepalive for Fennec. v1 Clearing review for now. Needs that fix and a clean try build.

I think gcp is right about comment 1. Let's doublecheck with safe browsing team at Google first though.

Monica, can you own finding that out?

(In reply to Richard Newman [:rnewman] from comment #14) > Monica, can you own finding that out? The answer is yes, we can delete the header. From Noe: I don't see any issues with that. In fact, other clients don't use keep-alive connections. Given that redirect URLs are hosted on a different host than the update server I don't think keep-alive is that useful in this case.

Thanks, Monica! Morphing this bug appropriately. Assuming we never want keepalive, this should turn into a two-line patch. (Next week, unless someone wants to steal it.)

This unconditionally specifies Connection: close on urlclassifier channels.

I needed one small change to fix one test failure: don't attempt to QI and add the header if we're doing a test (i.e., data: or file: URI), because they're not HTTP channels. There's still one failing test: test_hashcompleter fails if I specify Connection: close. 0:05.14 JavaScript error: , line 0: uncaught exception: 2147500036 0:05.14 JavaScript error: file:///Users/rnewman/moz/hg/fx-team/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/components/nsUrlClassifierHashCompleter.js, line 375: NS_ERROR_ABORT: Abort'Abort' when calling method: [nsIUrlClassifierHashCompleterCallback::completionFinished] 0:05.16 JavaScript strict warning: resource://testing-common/httpd.js, line 791: ReferenceError: reference to undefined property this._stopCallback 0:05.16 !!! error running onStopped callback: TypeError: callback is not a function Looks like the test server isn't dancing with the client correctly. Will investigate later.

filter on [mass-p5]

This is still waiting on me to fix the test.

Richard, can you update the latest version of your patch? This is too "easy" a win to let it die like this.

I meant "upload" not "update". I'm referring to the extra QI fix that comment 19 talks about.

Here's where I got to.

Also fixes a bug that could cause this test to fail intermittently in a parallel run.

Comment on attachment 8523859 [details] [diff] [review] Disable HTTP Keepalive for SafeBrowsing Review of attachment 8523859 [details] [diff] [review]: ----------------------------------------------------------------- If the tests pass, I'm happy! ::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js @@ +175,5 @@ > add: function HCR_add(aPartialHash, aCallback) { > this._requests.push({ > partialHash: aPartialHash, > callback: aCallback, > + responses: [] Naw, trailing commas are current style. Means you don't end up with a multi-line change for adding or removing a line.

(In reply to Richard Newman [:rnewman] from comment #26) > Naw, trailing commas are current style. Means you don't end up with a > multi-line change for adding or removing a line. Reference pointing out this is now allowed (as opposed to forbidden per ECMA-262, which is what my editor whined about): https://bugzilla.mozilla.org/show_bug.cgi?id=508637