Closed Bug 961616 Opened 6 years ago Closed 6 years ago

Simple HTTP2 Cookie crumbling unit tests

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gabor, Assigned: gabor)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131210132650
Assignee: nobody → gabor
Attached patch bug-961616-fix.patch (obsolete) — Splinter Review
Attachment #8365720 - Flags: review?(hurley)
The only problematic thing I found while writing the test case is this. Now the splitting into multiple header pairs works like this (equivalent JS code):

cookie_header_value.split('; ');

It would be probably more robust to do something like

cookie_header_value.split(';').trim();

There's a difference in cases like 'a=b;c=d' or 'a=b;  c=d'. But it's not a problem if Firefox always constructs the cookies header using '; ' as separator.
(In the second code snippet there should be a .map() but you get the idea I guess :) )
Comment on attachment 8365720 [details] [diff] [review]
bug-961616-fix.patch

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

Looks good, just a couple things to fix and I think we'll be ready to get this landed!

::: netwerk/test/unit/test_http2.js
@@ +125,5 @@
>  
>  Http2HeaderListener.prototype.onDataAvailable = function(request, ctx, stream, off, cnt) {
>    this.onDataAvailableFired = true;
>    this.isHttp2Connection = checkIsHttp2(request);
> +  this.callback(request.getResponseHeader(this.name))

Since this isn't just a do_check_eq anymore, you should ensure that the response header exists, so something like

var header = request.getResponseHeader(this.name);
do_check_neq(header, "");
this.callback(header);

@@ +254,5 @@
> +    }).map(function(pair) {
> +      return pair[1];
> +    }).sort().forEach(function(cookieReceived, index) {
> +        do_check_eq(cookiesSent[index], cookieReceived)
> +    });

It would be good to check that all the cookies sent are checked here. For example, it could be the case that a=b and c=d get sent, but e=f gets lost somewhere. If that were to happen, all your checks would succeed, and the test would pass, but the code would actually be broken :) Probably just keep track of how many times you call do_check_eq and at the end, ensure that number is equal to cookiesSent.length

::: testing/xpcshell/moz-http2/moz-http2.js
@@ +6,5 @@
>  var fs = require('fs');
>  var url = require('url');
>  var crypto = require('crypto');
>  
> +// Hooking into the decompression code to log the decompressed name-value pairs

nit: "Hook", not "Hooking"
Attachment #8365720 - Flags: review?(hurley)
Attached file bug-961616-fix v2 (obsolete) —
Attachment #8365720 - Attachment is obsolete: true
Attachment #8369203 - Flags: review?(hurley)
Thanks for the review, I've just made the requested modifications. How about this version?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=82a94864a176

Will this patch interfere with bug 964563?
Attached patch bug-961616-fix-v2.patch (obsolete) — Splinter Review
Attachment #8369203 - Attachment is obsolete: true
Attachment #8369203 - Flags: review?(hurley)
Attachment #8369205 - Attachment is obsolete: true
Attachment #8369208 - Flags: review?(hurley)
Attachment #8369208 - Flags: review?(hurley) → review+
(In reply to Gábor Molnár from comment #7)
> Will this patch interfere with bug 964563?

It shouldn't. And even if it does, that'll be my responsibility to figure out, since (with the amount of time the review on that one is taking) this one is going to land *way* earlier :)
Nick, could you outline the remaining steps for landing this on m-c? I couldn't find the exact process for this in the wiki after some time spent with searching.
Sorry. Normally, you just set the checkin-needed keyword, and one of the helpful sheriffs will get to it within a day or so. I went ahead and pushed it inbound for you, though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/45d655318813
https://hg.mozilla.org/mozilla-central/rev/45d655318813
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Thanks! ;)
You need to log in before you can comment on or make changes to this bug.