Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

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

Tracking

unspecified
mozilla35
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Posted patch bz1067679-1.patch (obsolete) — Splinter Review
Attachment #8489729 - Flags: review?(sworkman)
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?
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.
Posted 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.
Posted 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]
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.
Attachment #8491798 - Flags: review+
Posted 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): https://tbpl.mozilla.org/?tree=Try&rev=de973d2a8567 https://tbpl.mozilla.org/?tree=Try&rev=cc7b68beb4bb
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/moz.build
@@ +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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53343ea6245
https://hg.mozilla.org/mozilla-central/rev/4d21e0728967
https://hg.mozilla.org/mozilla-central/rev/f53343ea6245
Status: NEW → RESOLVED
Closed: 5 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/rules.mk:948: 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/rules.mk:948: recipe for target
> 'nsHostResolver.o' failed

https://bugzilla.mozilla.org/show_bug.cgi?id=1070966
Depends on: 1070966
I landed a trivial fixup for crosscompiling on case sensitive OSes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbff6a89854a
Flags: qe-verify-
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?
Depends on: 1123324
You need to log in before you can comment on or make changes to this bug.