Bug 55973

Summary: [Gtk] Provide a WebView knob to disable DNS prefetching
Product: WebKit Reporter: Marco Peereboom <marco>
Component: WebKit APIAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, christian, commit-queue, gustavo, todd, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
none
Proposed patch #2
none
diff against svn trunk rev 80650
none
diff against svn 80718
none
fixed diff against svn 80718
none
take 2 fixed diff against svn 80718
none
moved private browsing mention to docs not changelog .. against svn 80722
none
diff against 80853 with enable- prefix added as requested none

Description Marco Peereboom 2011-03-08 14:51:12 PST
Created attachment 85094 [details]
Proposed patch

The proposed patch adds a knob to enable/disable DNS prefeching.  This makes the browser quite a bit snappier and prevents DNS information leakage etc.

It basically is a backport of https://bugs.webkit.org/show_bug.cgi?id=28825
Comment 1 Christian Dywan 2011-03-08 16:36:43 PST
Can you elaborate on why disabling a feature that exists soley for performance reasons makes "the browser quite a bit snappier"?

How does DNS prefetching correlate with enable-private-browsing? It doesn't say.

dns-prefetch → dns-prefetching

Since: 1.2.8 → Since: 1.1.13

It might be nice to briefly explain what DNS prefetching does as it appears in the dcoumentation now.
Comment 2 Marco Peereboom 2011-03-09 03:47:36 PST
Created attachment 85155 [details]
Proposed patch #2
Comment 3 Marco Peereboom 2011-03-09 03:57:12 PST
Updated the patch with all your suggestions, thanks :-)

My non-scientific setup is as follows:
* OpenBSD 4.9
* WebKitGTK 1.2.7
* xxxterm browser 1.348
* adsuck 2.1 (DNS server that spoofs NXDOMAIN for unwanted servers)

I rigged the browser to display total page load time.  Where total page load is
defined as time spent between WEBKIT_LOAD_PROVISIONAL and WEBKIT_LOAD_FINISHED.

I ran it through a number of websites and got the following results:
prefetch 0:  page load time: 2.148816  ->   http://slashdot.org/
prefetch 1:  page load time: 7.705360  ->   http://slashdot.org/

prefetch 0:  page load time: 0.973012  ->   http://news.google.com/
prefetch 1:  page load time: 19.757806  ->   http://news.google.com/

prefetch 0:  page load time: 1.504187  ->   http://www.wired.com/
prefetch 1:  page load time: 3.965751  ->   http://www.wired.com/

prefetch 0:  page load time: 1.283165  ->   http://www.theregister.co.uk/
prefetch 1:  page load time: 1.759814  ->   http://www.theregister.co.uk/

prefetch 0:  page load time: 2.288612  ->   http://www.techreview.com/blog/arxiv/
prefetch 1:  page load time: 3.196511  ->   http://www.techreview.com/blog/arxiv/

prefetch 0:  page load time: 2.149060  ->   http://www.cnn.com/
prefetch 1:  page load time: 5.169388  ->   http://www.cnn.com/

prefetch 0:  page load time: 2.192299  ->   http://espn.go.com/
prefetch 1:  page load time: 7.610725  ->   http://espn.go.com/

That is what I mean by "quite a bit snappier".  The browser seems to load pages
at a linear speed instead of bumping along (by observing the completion bar).

This is just the performance aspect for me.  I can go into the security issues
as well but I think those have been described before in other bugs.
Comment 4 Alexey Proskuryakov 2011-03-09 10:22:33 PST
It's quite hard to test DNS prefetch impact. Once you access a site once, DNS records are cached on your computer, as well as on your provider's server, so tests aren't repeatable.

I believe that Google gets its numbers by enabling features like this for a certain percentage of Chrome users, and watching average page load speed, and their data suggested an improvement.

--- WebCore/dom/Document.cpp.orig	Fri Sep 10 08:20:33 2010
+++ WebCore/dom/Document.cpp	Tue Mar  8 10:16:45 2011

It's really weird to see branch patches that aren't made against any svn.webkit.org code.

         bool m_tiledBackingStoreEnabled : 1;
+	bool m_dnsPrefetchingEnabled : 1;

Tab here. Since the patch isn't made against current sources (and since it's also not marked for review), style bot won't automatically report problems.

+        priv->dns_prefetching = g_value_get_boolean(value);
+	break;

More tabs.

+        g_value_set_boolean(value, priv->dns_prefetching);
+	break;

Ditto.
Comment 5 Alexey Proskuryakov 2011-03-09 10:30:23 PST
I should also mention that there was a number of follow-up bugs about when DNS prefetch should be enabled after bug 28825. See bug 42500, bug 48813, bug 50953, bug 50996.
Comment 6 Todd T. Fries 2011-03-09 11:52:42 PST
Created attachment 85211 [details]
diff against svn trunk rev 80650
Comment 7 Marco Peereboom 2011-03-10 03:54:38 PST
Does Todd's diff meet all requirements?
Comment 8 Christian Dywan 2011-03-10 06:31:41 PST
(In reply to comment #6)
> Created an attachment (id=85211) [details]
> diff against svn trunk rev 80650

+        Reviewed no one yet
+
+        Provide a knob to enable/disable DNS prefetching.
+        DNS prefetching is enabled by default.
+        https://bugs.webkit.org/show_bug.cgi?id=28825

I recommend you try prepareChangeLog instead of writing it by hand.

And please keep the common style, ie:

Reviewed by ....

title of the bug
URL

description of the changes

I still think this should really mention that DNS prefetching is not affected private browsing because this is not at all obvious. Especially given the privacy-related concerns in other bugs which suggest that people may assume a relation.

Thanks a lot for providing the numbers, I personally think it's a reasonable motivation for the setting.
Comment 9 Todd T. Fries 2011-03-10 07:29:35 PST
Created attachment 85322 [details]
diff against svn 80718

provides a g_object property to toggle dns prefetching
Comment 10 WebKit Review Bot 2011-03-10 07:32:34 PST
Attachment 85322 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.cpp:3297:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/gtk/webkit/webkitwebview.cpp:3451:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Todd T. Fries 2011-03-10 07:42:40 PST
Created attachment 85325 [details]
fixed diff against svn 80718

        Provide a knob to enable/disable DNS prefetching.
        DNS prefetching is enabled by default.  This does not
        effect 'private browsing', it is an independent knob.
Comment 12 WebKit Review Bot 2011-03-10 07:45:30 PST
Attachment 85325 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.cpp:3297:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/gtk/webkit/webkitwebview.cpp:3451:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Todd T. Fries 2011-03-10 07:48:33 PST
Created attachment 85327 [details]
take 2 fixed diff against svn 80718

        Provide a knob to enable/disable DNS prefetching.
        DNS prefetching is enabled by default.  This does not
        effect 'private browsing', it is an independent knob.
Comment 14 Christian Dywan 2011-03-10 08:06:13 PST
(In reply to comment #13)
> Created an attachment (id=85327) [details]
> take 2 fixed diff against svn 80718

+        Provide a knob to enable/disable DNS prefetching.
+        DNS prefetching is enabled by default.  This does not
+        effect 'private browsing', it is an independent knob.
+        https://bugs.webkit.org/show_bug.cgi?id=28825

I meant to mention private browsing *in the documentation*, not the change log. So that users of the API actually see it. I should've been clearer.

The bug link is wrong.
Comment 15 Todd T. Fries 2011-03-10 08:37:27 PST
Created attachment 85336 [details]
moved private browsing mention to docs not changelog .. against svn 80722

        Provide a knob to enable/disable DNS prefetching.
        DNS prefetching is enabled by default.
        https://bugs.webkit.org/show_bug.cgi?id=55973
Comment 16 Todd T. Fries 2011-03-10 11:00:39 PST
http://www.pinkbike.com/news/DNS-Prefetching-implications.html

A definite negative side effect of dns prefetching.
Comment 17 Alexey Proskuryakov 2011-03-10 11:34:17 PST
I don't think that the aforelinked article impacts this patch in any way.

From mailing list discussion, it sounds like Gtk may need to have DNS prefetch completely disabled, at least on some platforms, and not a preference.
Comment 18 Todd T. Fries 2011-03-10 11:56:53 PST
Alexey, I think you are missing the point.  The ability to toggle this at will permits us to determine if it is indeed good or bad in a given situation.  Senslessly hardcoding things is why this patch was generated in the first place, to give the user and/or application the choice, not hardcoding webkit.  This is a user preference on both windows and mac, where the majority of the webkit usage remains in terms of browsers on systems.  Please let gtk follow in the same footsteps and let us have a knob to make the choice as well.
Comment 19 Alexey Proskuryakov 2011-03-10 12:11:34 PST
> Please let gtk follow in the same footsteps and let us have a knob to make the choice as well.

It's not true that this is a user preference in Safari. It is a WebKit preference indeed, but it's not exposed to users in Safari UI.

Adding a method in API to toggle something that you know is always bad is a rather strange thing to do. API methods are a permanent liability - you can't take it out once you add it.

But anyway, I'm not stopping anyone here. I'm certainly not in position for making API decisions for Gtk.
Comment 20 Marco Peereboom 2011-03-11 04:32:42 PST
Is there anything else missing in this patch?
Comment 21 Gustavo Noronha (kov) 2011-03-11 07:11:56 PST
I agree with Alexey that this should not be exposed (at least in Epiphany) to the user, but having a webkit preference makes sense to me, and though I still feel that it makes more sense as a build-time option, I'm not opposed to having it as a runtime option. It should be renamed enable-dns-prefetch to match the convention used in other similar preferences, though. In WebKitGTK+ we adopt a policy to have at least two GTK+ reviewers agree to a new API addition before commiting to it. You got my agreement, provided that we rename it, so if Martin Robinson or Xan agree, we can land this =).
Comment 22 Todd T. Fries 2011-03-11 07:38:36 PST
Created attachment 85474 [details]
diff against 80853 with enable- prefix added as requested

        Provide a knob to enable/disable DNS prefetching.
        DNS prefetching is enabled by default.
        https://bugs.webkit.org/show_bug.cgi?id=55973
Comment 23 Gustavo Noronha (kov) 2011-03-11 07:44:25 PST
Comment on attachment 85474 [details]
diff against 80853 with enable- prefix added as requested

OK, looking good now. Martin expressed he's ok with this on the mailing list and xan said 'why not', so let's do it!
Comment 24 Marco Peereboom 2011-03-11 07:45:30 PST
w00t!  thanks everybody!
Comment 25 WebKit Commit Bot 2011-03-11 09:28:11 PST
Comment on attachment 85474 [details]
diff against 80853 with enable- prefix added as requested

Clearing flags on attachment: 85474

Committed r80856: <http://trac.webkit.org/changeset/80856>
Comment 26 WebKit Commit Bot 2011-03-11 09:28:17 PST
All reviewed patches have been landed.  Closing bug.