Closed Bug 942317 Opened 6 years ago Closed 6 years ago

Experiment to test reachability of SRV records

Categories

(Core :: Networking: DNS, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- disabled

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch dns_experiment.patch (obsolete) — Splinter Review
See how feasible it is to use this as an upgrade path from http/1 to http/2. Windows-only, requires server-side support, as well.

Try run for existing patch at https://tbpl.mozilla.org/?tree=Try&rev=2b66bc998c61 (I wouldn't be surprised if some test somewhere is broken by this, but it at least worked on my Win7 VM)
Attachment #8337041 - Flags: feedback?(mcmanus)
Depends on: 943359
Comment on attachment 8337041 [details] [diff] [review]
dns_experiment.patch

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

::: netwerk/dns/nsHostResolver.cpp
@@ +887,5 @@
> +        endALookup = mozilla::TimeStamp::Now();
> +
> +        // SRV lookup
> +        startSRVLookup = mozilla::TimeStamp::Now();
> +        srvStatus = dnsQuery(srvName.get(), DNS_TYPE_SRV, DNS_QUERY_STANDARD,

I think the A and SRV need to be done in parallel.. its unclear if there will need to be multiple round trips to pick up the delegations of org and mozilla and aus and its not fair to make the first lookup pay the cost of that and have the second lookup get the cache hit on it.

::: toolkit/components/telemetry/Histograms.json
@@ +1729,5 @@
> +  "SRV_EXPERIMENT_SUCCESS_DELTA": {
> +      "kind": "linear",
> +      "high": "2000",
> +      "n_buckets": 200,
> +      "description": "Time delta between A and SRV records when both succeed"

description should indicate the offset by 1k magic
Attachment #8337041 - Flags: feedback?(mcmanus)
also should have mentioned that the test should verify the expected return values when it succeeds.
Attached patch dns_experiment.patch (obsolete) — Splinter Review
New version, all feedback comments addressed. Try seems unreasonably happy (which always kind of worries me), but hey, It Works On My Machine!(TM)

https://tbpl.mozilla.org/?tree=Try&rev=1685d80b2793
Attachment #8337041 - Attachment is obsolete: true
Attachment #8342068 - Flags: review?(mcmanus)
Comment on attachment 8342068 [details] [diff] [review]
dns_experiment.patch

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

sweet 

needs to be conditional on 
1] pref network.allow-experiments 
2] Telemetry::CanRecord()
3] a new kill switch just for this feature. default to true for EARLY_BETA_OR_EARLIER, false otherwise

::: netwerk/dns/nsHostResolver.cpp
@@ +846,5 @@
> +    NS_IMETHOD Run() MOZ_OVERRIDE
> +    {
> +        nsAutoCString name;
> +        if (mQueryType == DNS_TYPE_SRV) {
> +            name.Assign(NS_LITERAL_CSTRING("srv-"));

"_http2tls.srv-"

@@ +974,5 @@
> +                                                          &srvResults,
> +                                                          dnsQuery),
> +                                   NS_DISPATCH_NORMAL);
> +
> +        WaitForMultipleObjects(2, events, TRUE, INFINITE);

mad windows skillz

@@ +985,5 @@
> +        }
> +
> +        if (srvStatus == DNS_RCODE_NOERROR) {
> +            DNS_SRV_DATA *srvData = &srvResults->Data.Srv;
> +            if (strcmp(srvData->pNameTarget, "success.http2test.mozilla.org") ||

strcasecmp

@@ +1066,5 @@
> +        MutexAutoLock lock(mExperimentLock);
> +        if (mHasRunExperiment) {
> +            return;
> +        }
> +

so the threading rules around the dns library are kind of squishy because it is pretty well locked. I think your new code requires the main thread (ns_newnamedthread() likely does).. so let's just do if (!IsMainThread()) return; right here in runexperiment().

@@ +1087,5 @@
>  {
>      MOZ_EVENT_TRACER_WAIT(rec, "net::dns::resolve");
>  
> +#if defined(XP_WIN)
> +    if (strcmp(rec->host, "aus.mozilla.org") == 0) {

I think we can do "ends in .mozilla.org" instead.. that might get us more data. Also preface the string operation with a racy if (enabled && !mHasRunExperiment) check.. (enabled based on the prefs) you'll need to do it again inside the lock, but that will avoid the string op and lock acquisition the vast majority of the time and always on release.
Attachment #8342068 - Flags: review?(mcmanus)
and stick a reference to this bug # in a comment to help with any wtf moments.
All comments addressed. Fun fact: strcasecmp doesn't exist on windows, but _stricmp does. /me sighs
Attachment #8342068 - Attachment is obsolete: true
Attachment #8342679 - Flags: review?(mcmanus)
Comment on attachment 8342679 [details] [diff] [review]
dns_experiment.patch

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

lgtm. exciting!
Attachment #8342679 - Flags: review?(mcmanus) → review+
green try run: https://tbpl.mozilla.org/?tree=Try&rev=f8c210a749c6 (combine with try run from comment 3 for full greenness, since none of this code is even built outside windows)

Just waiting on inbound to open...
And inbound is open: remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0128d154ed3
https://hg.mozilla.org/mozilla-central/rev/c0128d154ed3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Trivial fix for cross compilation on case sensitive OSes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/316ab101c526
Depends on: 950888
Blocks: 957759
(disabled in 28 and later in bug 957759)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.