Closed Bug 1159944 Opened 5 years ago Closed 5 years ago

more alt-svc tests

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Whiteboard: [spdy])

Attachments

(1 file)

among other things, this patch changes moz-http2 to use a signed cert
for foo.example.com and it shows how to setup an xpcshell profile
with the ca cert used to sign it. We should use that to write a test
for JoinConnection/Pooling.
Attachment #8599590 - Flags: review?(hurley)
Whiteboard: [spdy]
Blocks: 1159971
Comment on attachment 8599590 [details] [diff] [review]
more alt-svc tests

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

Looks pretty good, though special care may want to be taken around my last comment below - I'm totally not understanding what's going on there, and while it's quite likely I'm missing something, at least an explanation of what that is would be good (both in bugzilla, and in a comment in the code, for future reference). r=me with changes noted here and below.

::: netwerk/test/unit/test_altsvc.js
@@ +46,5 @@
> +  prefs.setBoolPref("network.http.altsvc.oe", true);
> +  prefs.setCharPref("network.dns.localDomains", "foo.example.com, bar.example.com");
> +
> +  var oldPref = prefs.getIntPref("network.http.speculative-parallel-limit");
> +  prefs.setIntPref("network.http.speculative-parallel-limit", 0);

I'm not getting the point of this (and then resetting the value a few lines down) when it doesn't appear that any HTTP stuff is happening between here and when the value gets reset to its original value. Am I missing something, or is this just left over from some previous iteration of the patch? (Seems like it might be leftover/copy-paste from the old cert exception adding bits in the h2 and spdy tests.)

@@ +98,5 @@
> +    var hval = "h2=" + metadata.getHeader("x-altsvc");
> +    response.setHeader("Alt-Svc", hval, false);
> +  } catch (e) {}
> +
> +  var body = "Q: What did 0 say to 8? A: Nice Belt!\n";

ba-dum ksshhhhh

@@ +158,5 @@
> +      if (!Components.isSuccessCode(request.status)) {
> +        do_throw("Channel should have a success code! (" + request.status + ")");
> +      }
> +      do_check_eq(request.responseStatus, 200);
> +     } else {

nit: hard to tell in splinter, but the indentation looks off here (looks like it should be de-indented one more space)

@@ +228,5 @@
> +  xaltsvc = h2Route;
> +  nextTest = doTest2;
> +  do_test_pending();
> +  doTest();
> +  xaltsvc = h2FooRoute;

I'm not seeing how this assignment here is necessary, or even correct. And yet, your xpcshell runs are passing (though without any dump()s showing up - is that expected on CI?)
Attachment #8599590 - Flags: review?(hurley) → review+
Comment on attachment 8599590 [details] [diff] [review]
more alt-svc tests

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

::: netwerk/test/unit/test_altsvc.js
@@ +46,5 @@
> +  prefs.setBoolPref("network.http.altsvc.oe", true);
> +  prefs.setCharPref("network.dns.localDomains", "foo.example.com, bar.example.com");
> +
> +  var oldPref = prefs.getIntPref("network.http.speculative-parallel-limit");
> +  prefs.setIntPref("network.http.speculative-parallel-limit", 0);

good catch - that's left from when it handled the cert like test_http2.js and can be removed

@@ +228,5 @@
> +  xaltsvc = h2Route;
> +  nextTest = doTest2;
> +  do_test_pending();
> +  doTest();
> +  xaltsvc = h2FooRoute;

yeah, I should have commented on this.

xaltsvc is overloaded to do two things.. 
1] it is sent in the x-altsvc request header, and the response uses the value in the Alt-Svc response header
2] the test polls until necko sets Alt-Used to that value (i.e. it uses that route)

in doTest1(), the Alt-Svc is the ":443" production with the implied origin.. but the Alt-Used is never an implied origin so it has to be fixed up after the channel with the x-altsvc is sent.
https://hg.mozilla.org/mozilla-central/rev/35dfb84cc70f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.