888583 - Expose http/1 version numbers in about:networking

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Robert Bindar]

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

Comment 1

[Robert Bindar]

Attachment: patch v1

Comment 2

[Robert Bindar]

added some spaces for keeping the integrity of the module.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 769345 [details] [diff] [review]
patch v1

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

::: dom/webidl/NetDashboard.webidl
@@ +17,5 @@
>  dictionary HttpConnInfoDict {
>  	sequence<unsigned long> rtt;
>  	sequence<unsigned long> ttl;
>    sequence<octet> spdyVersion;
> +  sequence<octet> httpVersion;

indentation

::: netwerk/base/src/DashboardTypes.h
@@ +39,1 @@
>  };

it isn't possible to simultaneously have an http/1 version and a spdy version (spdy is really the prototype of http/2).. to make this future proof I'd like to see it collapsed to just one field that supports all 5 protocols (0.9, 1.0, 1.1, spdy/2, spdy/3) as well as future ones

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +68,5 @@
>      , mUsingSpdyVersion(0)
>      , mPriority(nsISupportsPriority::PRIORITY_NORMAL)
>      , mReportedSpdy(false)
>      , mEverUsedSpdy(false)
> +    , mHttpVersion(NS_HTTP_VERSION_1_1)

please name this mLastHttpResponseVersion

::: netwerk/protocol/http/nsHttpConnection.h
@@ +118,5 @@
>      nsresult PushBack(const char *data, uint32_t length);
>      nsresult ResumeSend();
>      nsresult ResumeRecv();
> +    int64_t  MaxBytesRead() { return mMaxBytesRead; }
> +    uint8_t GetHttpVersion() { return mHttpVersion; }

naming again..

Comment 5

[Robert Bindar]

Attachment: patch v2

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 769405 [details] [diff] [review]
patch v2

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

you've added a requirement that the enums of SPDY_VERSION and NS_HTTP_VERSION not overlap.. that's not really workable because

1] NS_HTTP_VERSIONS are sometimes compared, so later versions need higher numbers.. and VERSION_1_0 is defined as 10 but SPDY_VERSION_2 is set to 2
2] SPDY_VERSION_2 can't be set to something new (like 20) because it is used as an index in telemetry

Comment 7

[Valentin Gosu [:valentin] (he/him)]

As I see it, we have a few options of handling this:
1. Only hold protocol version as a string. This way we bypass the issue with comparing protocol versions.
2. Create a new class for the protocol version and overload the > operator. This seems like a really unsafe thing to do (I don't usually expect that 2 > 10 == true )
3. Make the protocolVersion in DashboardTypes.h private to the Dashboard so that people don't accidentally use this value.

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu from comment #7)
> As I see it, we have a few options of handling this:
> 1. Only hold protocol version as a string. This way we bypass the issue with
> comparing protocol versions.
> 2. Create a new class for the protocol version and overload the > operator.
> This seems like a really unsafe thing to do (I don't usually expect that 2 >
> 10 == true )
> 3. Make the protocolVersion in DashboardTypes.h private to the Dashboard so
> that people don't accidentally use this value.


Current comparisons are ok because they are restricted in scope. (http/1 minors vs each other, spdy revs vs each other).. I'm just worried about creep.

Can the dashboard just keep it as a string in its class? It would clearly make better output :)

Comment 9

[Robert Bindar]

Attachment: patch v3

I've added the protocol version as a string in dashboard.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 770891 [details] [diff] [review]
patch v3

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

getting closer!

::: netwerk/base/src/Dashboard.cpp
@@ +12,5 @@
> +#define NS_HTTP_VERSION_0_9 9
> +#define NS_HTTP_VERSION_1_0 10
> +#define NS_HTTP_VERSION_1_1 11
> +#define SPDY_VERSION_2 2
> +#define SPDY_VERSION_3 3

if you end up needing them after revising this, dont redefine them.. use them via include

@@ +455,5 @@
> +HttpConnInfo::SetProtocolVersion(uint8_t pv)
> +{
> +    switch (pv) {
> +    case SPDY_VERSION_2:
> +        protocolVersion = "spdy/2";

protocolVersion.Assign(NS_LITERAL_CSTRING("spdy/2"));

::: netwerk/base/src/DashboardTypes.h
@@ +33,5 @@
>  struct HttpConnInfo
>  {
>      uint32_t ttl;
>      uint32_t rtt;
> +    nsCString protocolVersion;

This is really for JS, right? So It should be a nsString so you don't need the UTF16 copy conversion later.. might as well just do the assignment in utf16.

@@ +35,5 @@
>      uint32_t ttl;
>      uint32_t rtt;
> +    nsCString protocolVersion;
> +
> +    void SetProtocolVersion(uint8_t pv);

I would get rid of setProtocolVersion and create two functions - SetHTTP1ProtocolVersion and setHTTP2ProtocolVersion (spdy can use this one) and then the connection manager can use the right one given the context it already knows.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 770891 [details] [diff] [review]
patch v3

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

::: netwerk/base/src/Dashboard.cpp
@@ +12,5 @@
> +#define NS_HTTP_VERSION_0_9 9
> +#define NS_HTTP_VERSION_1_0 10
> +#define NS_HTTP_VERSION_1_1 11
> +#define SPDY_VERSION_2 2
> +#define SPDY_VERSION_3 3

NS_HTTP_VERSION_* you can get by importing nsHttp.h
However, ASpdySession.h isn't exported by moz.build
You can either export it, redefine the enum, or move the enum to a common header (maybe even nsHttp.h)

Comment 12

[Robert Bindar]

Attachment: Moving Spdy versions enum to nsHtpp.h

Comment 13

[Robert Bindar]

Attachment: Expose protocol version

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 771235 [details] [diff] [review]
Moving Spdy versions enum to nsHtpp.h

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

r=mcmanus with the changes I note and a successful try run on linux first please

::: netwerk/protocol/http/nsHttp.h
@@ +22,5 @@
>  #define NS_HTTP_VERSION_1_1     11
>  
> +namespace mozilla {
> +namespace net {
> +namespace Spdy {

just mozilla::net please

@@ +27,5 @@
> +    enum {
> +        SPDY_VERSION_2 = 2,
> +        SPDY_VERSION_3 = 3
> +    };
> +} } } // namespace mozilla::net::Spdy

each brace on its own line

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

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

I'm good with most of this.. valentin can be the final arbiter

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

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

Looks good. r=me

Comment 17

[Robert Bindar]

Attachment: Move Spdy versions enum to nsHtpp.h

r=mcmanus

Comment 18

[Robert Bindar]

Please check in patch 771997 and patch 771236.
https://tbpl.mozilla.org/?tree=Try&rev=4ea4b43a67ef

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage on multiple platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee826796cd0f

https://tbpl.mozilla.org/php/getParsedLog.php?id=25013788&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25013848&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25013937&tree=Mozilla-Inbound

Comment 20

[Patrick McManus [:mcmanus]]

robert, also please actually run the tests on your next try run :) (you just built it)

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

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

::: netwerk/base/src/Dashboard.cpp
@@ +464,5 @@
> +
> +void
> +HttpConnInfo::SetHTTP2ProtocolVersion(uint8_t pv)
> +{
> +    if (pv == Spdy::SPDY_VERSION_2)

This is no longer in the Spdy namespace in the latest patch. Please fix.

Comment 22

[Robert Bindar]

sorry, my bad, I've forgotten to update the second patch too. I will do it now.

Comment 23

[Robert Bindar]

Attachment: expose protocol version

r=valentin.gosu

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Patch 2 needs rebasing against m-c/inbound. Also, judging by comment 20, a Try run with tests run would also be appreciated :)

Comment 25

[Robert Bindar]

Attachment: expose protocol version

r=valentin.gosu

Comment 26

[Robert Bindar]

I will come soon with a try tun with tests.

Comment 27

[Robert Bindar]

Attachment: Expose protocol version

r=valentin.gosu

Comment 28

[Robert Bindar]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79fcaa76d774
https://hg.mozilla.org/integration/mozilla-inbound/rev/346e8cd551af

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/79fcaa76d774
https://hg.mozilla.org/mozilla-central/rev/346e8cd551af