Status

()

Core
Networking: DNS
RESOLVED FIXED
4 years ago
3 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)

(Assignee)

Description

4 years ago
We want to run an experiment to see how useful using the TTL will be. We can get to it by using DnsQuery.
(Assignee)

Updated

4 years ago
Assignee: nobody → josullivan
(Assignee)

Comment 1

4 years ago
Created attachment 8489729 [details] [diff] [review]
bz1067679-1.patch
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
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8491798 [details] [diff] [review]
Changes.patch

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

Comment 7

4 years ago
Created attachment 8491800 [details] [diff] [review]
bz1067679-2.patch

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+
(Assignee)

Comment 9

4 years ago
Created attachment 8492311 [details] [diff] [review]
Bug 1067679.patch
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)
(Assignee)

Comment 10

4 years ago
Created attachment 8492318 [details] [diff] [review]
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
Attachment #8492318 - Flags: review?(sworkman)
(Assignee)

Updated

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 16

4 years ago
/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

Updated

4 years ago
Depends on: 1070966

Comment 18

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

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

Comment 20

3 years ago
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?

Updated

3 years ago
Depends on: 1123324
Depends on: 1190502
You need to log in before you can comment on or make changes to this bug.