Closed Bug 1031160 Opened 5 years ago Closed 5 years ago

Disable keep-alive for connections to safe browsing API

Categories

(Toolkit :: Safe Browsing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
fennec 36+ ---

People

(Reporter: m_kato, Assigned: gcp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
Summary: Add Pref to disable keep-alive for accessing safe browsing site such as Google safe browsing server → Add Pref to disable keep-alive for accessing safe browsing site such as Google safe browsing server etc
Blocks: powah
tracking-fennec: --- → ?
Hardware: ARM → All
Summary: Add Pref to disable keep-alive for accessing safe browsing site such as Google safe browsing server etc → Add pref to disable keep-alive for accessing safe browsing site such as Google safe browsing server etc
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.
tracking-fennec: ? → +
Untested, but this looks like it might be the answer.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Now with correct import.
Attachment #8491156 - Attachment is obsolete: true
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!
Attachment #8491147 - Flags: review?(dcamp)
Attachment #8491161 - Attachment is obsolete: true
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.
Flags: needinfo?(mmc)
(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.
Attachment #8491147 - Flags: review?(dcamp)
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.
Flags: needinfo?(mmc)
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.)
Summary: Add pref to disable keep-alive for accessing safe browsing site such as Google safe browsing server etc → Disable keep-alive for connections to safe browsing API
This unconditionally specifies Connection: close on urlclassifier channels.
Attachment #8493804 - Flags: review?(dcamp)
Attachment #8491147 - Attachment is obsolete: true
Attachment #8491168 - Attachment is obsolete: true
Attachment #8491168 - Flags: review?(dcamp)
Attachment #8493804 - Flags: review?(dcamp) → review+
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]
Priority: -- → P5
This is still waiting on me to fix the test.
tracking-fennec: + → 37+
Priority: P5 → --
Richard, can you update the latest version of your patch? This is too "easy" a win to let it die like this.
Flags: needinfo?(rnewman)
I meant "upload" not "update". I'm referring to the extra QI fix that comment 19 talks about.
Here's where I got to.
Attachment #8523152 - Flags: review?(gpascutto)
Attachment #8493804 - Attachment is obsolete: true
Also fixes a bug that could cause this test to fail intermittently in
a parallel run.
Attachment #8523859 - Flags: review?(rnewman)
Attachment #8523152 - Attachment is obsolete: true
Attachment #8523152 - Flags: review?(gpascutto)
Assignee: rnewman → gpascutto
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.
Attachment #8523859 - Flags: review?(rnewman) → review+
(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
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/83d3b89ca16c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
tracking-fennec: 37+ → 36+
You need to log in before you can comment on or make changes to this bug.