Bug 55973 – [Gtk] Provide a WebView knob to disable DNS prefetching

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Marco Peereboom]

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

[Christian Dywan]

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.

[Marco Peereboom]

Created attachment 85155 [details]
Proposed patch #2

[Marco Peereboom]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Todd T. Fries]

Created attachment 85211 [details]
diff against svn trunk rev 80650

[Marco Peereboom]

Does Todd's diff meet all requirements?

[Christian Dywan]

(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.

[Todd T. Fries]

Created attachment 85322 [details]
diff against svn 80718

provides a g_object property to toggle dns prefetching

[WebKit Review Bot]

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.

[Todd T. Fries]

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.

[WebKit Review Bot]

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.

[Todd T. Fries]

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.

[Christian Dywan]

(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.

[Todd T. Fries]

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

[Todd T. Fries]

http://www.pinkbike.com/news/DNS-Prefetching-implications.html

A definite negative side effect of dns prefetching.

[Alexey Proskuryakov]

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.

[Todd T. Fries]

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.

[Alexey Proskuryakov]

> 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.

[Marco Peereboom]

Is there anything else missing in this patch?

[Gustavo Noronha (kov)]

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 =).

[Todd T. Fries]

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

[Gustavo Noronha (kov)]

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!

[Marco Peereboom]

w00t!  thanks everybody!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.