1067679 - TTL Experiment

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[johnsullivan.pem]

We want to run an experiment to see how useful using the TTL will be. We can get to it by using DnsQuery.

Comment 1

[johnsullivan.pem]

Attachment: bz1067679-1.patch

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8489729 [details] [diff] [review]
bz1067679-1.patch

Review of attachment 8489729 [details] [diff] [review]:
-----------------------------------------------------------------

Already gone over this with you in person. Looks good to me. Just a couple of minor things. Otherwise, let's try it. r=me.

::: browser/app/profile/firefox.js
@@ +1734,5 @@
>  
>  // Enable the OpenH264 plugin support in the addon manager.
>  pref("media.gmp-gmpopenh264.provider.enabled", true);
> +
> +pref("dns.ttl-experiment.variant", 0);

// Experiment: Get TTL from DNS records.
// Unset initially (0); Randomly chosen on first run; will remain unchanged unless adjusted by the user or experiment ends.
// Variants defined in nsHostResolver.cpp.

::: netwerk/dns/DNS.h
@@ +142,5 @@
>  
>    char *mHostName;
>    char *mCanonicalName;
> +  uint16_t ttl;
> +  static const uint16_t NO_TTL_DATA = (uint16_t) -1;

I don't see ttl being set to NO_TTL_DATA anywhere in this patch. Can you add it to the constructor please?

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

John, can you also post a good try run here please? Mochitests and xpcshell should be good. Let's hit all the platforms just to make sure.

Comment 4

[Patrick McManus [:mcmanus]]

wow.. that's a big patch.

can you summarize what we're going to learn from it and how (if at all - maybe its a background activity) the experiment will impact resolution for its users?

Also make sure its disabled for release builds and whenever the umbrella network.allow-experiments is false

Comment 5

[johnsullivan.pem]

You should see the DnsQuery patch, that ones even bigger.

We'll learn if using the TTL to influence the expiration of each record is beneficial/harmful. The resolution time for medium and low priority requests will be negatively impacted (which will be mostly prefetches), because of the second lookup. High priority requests will not be impacted, because the TTL resolution is done in the background after the lookup is completed.

Comment 6

[johnsullivan.pem]

Attachment: Changes.patch

These are the changes since the last patch, provided for convenience. Will upload an attachment with everything together in a moment.

Comment 7

[johnsullivan.pem]

Attachment: bz1067679-2.patch

This is making its way through try right now.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8491798 [details] [diff] [review]
Changes.patch

Review of attachment 8491798 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look fine. Let's see how try does.

::: netwerk/dns/nsHostResolver.cpp
@@ +628,5 @@
> +
> +static void
> +DnsExperimentChanged(const char* aPref, void*)
> +{
> +    DebugOnly<nsresult> rv = NS_DispatchToMainThread(

Add a strcmp here to filter out the prefs you don't want.

Comment 9

[johnsullivan.pem]

Attachment: Bug 1067679.patch

Comment 10

[johnsullivan.pem]

Attachment: Bug 1067679 - Squashed.patch

Sent through try (the if statement to filter on prefs wasn't in these, but that change is very minor and I did local testing): https://tbpl.mozilla.org/?tree=Try&rev=de973d2a8567 https://tbpl.mozilla.org/?tree=Try&rev=cc7b68beb4bb

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8492318 [details] [diff] [review]
Bug 1067679 - Squashed.patch

Review of attachment 8492318 [details] [diff] [review]:
-----------------------------------------------------------------

Cool. Let's try it out!

::: netwerk/dns/GetAddrInfo.h
@@ +9,5 @@
> +
> +#include "nsError.h"
> +#include "nscore.h"
> +
> +#if defined(XP_WIN) && !defined(RELEASE_BUILD)

Nice: blocked from release for now.

::: netwerk/dns/moz.build
@@ +25,4 @@
>  ]
>  
>  SOURCES += [
> +    'GetAddrInfo.cpp',

nit: # Excluded from UNIFIED_SOURCES due to NSPR forced logging.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

John's most recent try run, all green: https://tbpl.mozilla.org/?tree=Try&rev=894093d680ab

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d21e0728967

Comment 14

[(Away)]

Visual Studio 2013 requires <algorithm> for std::min. I pushed a trivial fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53343ea6245

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d21e0728967
https://hg.mozilla.org/mozilla-central/rev/f53343ea6245

Comment 16

[Octoploid]

/var/tmp/mozilla-central/netwerk/dns/nsHostResolver.cpp:35:39: fatal error: mtransport/runnable_utils.h: No such file or directory
compilation terminated.
/var/tmp/mozilla-central/config/rules.mk:948: recipe for target 'nsHostResolver.o' failed

Comment 17

[Patrick McManus [:mcmanus]]

(In reply to Octoploid from comment #16)
> /var/tmp/mozilla-central/netwerk/dns/nsHostResolver.cpp:35:39: fatal error:
> mtransport/runnable_utils.h: No such file or directory
> compilation terminated.
> /var/tmp/mozilla-central/config/rules.mk:948: recipe for target
> 'nsHostResolver.o' failed

https://bugzilla.mozilla.org/show_bug.cgi?id=1070966

Comment 18

[Jacek Caban]

I landed a trivial fixup for crosscompiling on case sensitive OSes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbff6a89854a

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbff6a89854a

Comment 20

[Kai]

Using network-supplied DNS TTL opens the browser to DNS rebinding attacks.

Please see this 2007 paper: http://crypto.stanford.edu/dns/dns-rebinding.pdf

FF35.0.1 seems to ship with dns.ttl-experiment.enabled=true and network.dnsCacheExpiration=60 , but it's unclear to me whether this is still running in "survey only" mode.

Have efforts have been made to force DNS pinning in such a way that ALL plugins are protected from this?