Closed Bug 1067679 Opened 10 years ago Closed 10 years ago

TTL Experiment


(Core :: Networking: DNS, defect)

Windows 7
Not set





(Reporter: johnsullivan.pem, Assigned: johnsullivan.pem)




(1 file, 4 obsolete files)

We want to run an experiment to see how useful using the TTL will be. We can get to it by using DnsQuery.
Assignee: nobody → josullivan
Attached patch bz1067679-1.patch (obsolete) — Splinter Review
Attachment #8489729 - Flags: review?(sworkman)
Comment on attachment 8489729 [details] [diff] [review]

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?
Attachment #8489729 - Flags: review?(sworkman) → review+
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.
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
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.
Attached patch Changes.patch (obsolete) — Splinter Review
These are the changes since the last patch, provided for convenience. Will upload an attachment with everything together in a moment.
Attached patch bz1067679-2.patch (obsolete) — Splinter Review
This is making its way through try right now.
Attachment #8491800 - Flags: review?(sworkman)
Comment on attachment 8491798 [details] [diff] [review]

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.
Attachment #8491798 - Flags: review+
Attached patch Bug 1067679.patch (obsolete) — Splinter Review
Attachment #8489729 - Attachment is obsolete: true
Attachment #8491798 - Attachment is obsolete: true
Attachment #8491800 - Attachment is obsolete: true
Attachment #8491800 - Flags: review?(sworkman)
Attachment #8492311 - Flags: review?(sworkman)
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):
Attachment #8492318 - Flags: review?(sworkman)
Attachment #8492311 - Attachment is obsolete: true
Attachment #8492311 - Flags: review?(sworkman)
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/
@@ +25,4 @@
>  ]
>  SOURCES += [
> +    'GetAddrInfo.cpp',

nit: # Excluded from UNIFIED_SOURCES due to NSPR forced logging.
Attachment #8492318 - Flags: review?(sworkman) → review+
Visual Studio 2013 requires <algorithm> for std::min. I pushed a trivial fix:
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
/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/ recipe for target 'nsHostResolver.o' failed
(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/ recipe for target
> 'nsHostResolver.o' failed
Depends on: 1070966
I landed a trivial fixup for crosscompiling on case sensitive OSes:
Flags: qe-verify-
Using network-supplied DNS TTL opens the browser to DNS rebinding attacks.

Please see this 2007 paper:

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?
Depends on: 1123324
Depends on: 1190502
You need to log in before you can comment on or make changes to this bug.