961616 - Simple HTTP2 Cookie crumbling unit tests

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Gábor Molnár]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131210132650

Comment 1

[Gábor Molnár]

Attachment: bug-961616-fix.patch

Comment 2

[Gábor Molnár]

https://tbpl.mozilla.org/?tree=Try&rev=9f9cb65491cc

Comment 3

[Gábor Molnár]

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.

Comment 4

[Gábor Molnár]

(In the second code snippet there should be a .map() but you get the idea I guess :) )

Comment 5

[u408661]

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"

Comment 6

[Gábor Molnár]

Attachment: bug-961616-fix v2

Comment 7

[Gábor Molnár]

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?

Comment 8

[Gábor Molnár]

Attachment: bug-961616-fix-v2.patch

Comment 9

[Gábor Molnár]

Attachment: bug-961616-fix-v2.patch

Comment 10

[u408661]

(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 :)

Comment 11

[Gábor Molnár]

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.

Comment 12

[u408661]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/45d655318813

Comment 14

[Gábor Molnár]

Thanks! ;)