Disable keep-alive for connections to safe browsing API

RESOLVED FIXED in mozilla36

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: m_kato, Assigned: gcp)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
All
Android
Points:
---

Firefox Tracking Flags

(fennec36+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
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: 979119
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
(Assignee)

Comment 1

3 years ago
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.

Updated

3 years ago
tracking-fennec: ? → +
Created attachment 8491147 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec. v1

Untested, but this looks like it might be the answer.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Created attachment 8491156 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec (C++ part). v1

Oh hey, more code.
Created attachment 8491161 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec (C++ part). v2

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)
Created attachment 8491168 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec (C++ part). v3
Attachment #8491168 - Flags: review?(dcamp)
Attachment #8491161 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
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?
(Assignee)

Comment 9

3 years ago
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
Created attachment 8493804 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec. v3

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)
(Assignee)

Updated

3 years ago
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 → --
(Assignee)

Comment 22

3 years ago
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)
(Assignee)

Comment 23

3 years ago
I meant "upload" not "update". I'm referring to the extra QI fix that comment 19 talks about.
Created attachment 8523152 [details] [diff] [review]
Disable safebrowsing keepalive for Fennec. v4

Here's where I got to.
Attachment #8523152 - Flags: review?(gpascutto)
Attachment #8493804 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8523859 [details] [diff] [review]
Disable HTTP Keepalive for SafeBrowsing

Also fixes a bug that could cause this test to fail intermittently in
a parallel run.
Attachment #8523859 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8523152 - Attachment is obsolete: true
Attachment #8523152 - Flags: review?(gpascutto)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 27

3 years ago
(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
Last Resolved: 3 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.