Expose http/1 version numbers in about:networking

RESOLVED FIXED in mozilla25

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Robert Bindar, Assigned: Robert Bindar)

Tracking

21 Branch
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 769345 [details] [diff] [review]
patch v1
Attachment #769345 - Flags: review?(mcmanus)
(Assignee)

Comment 2

4 years ago
added some spaces for keeping the integrity of the module.
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..
Attachment #769345 - Flags: review?(mcmanus)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 888267
(Assignee)

Comment 5

4 years ago
Created attachment 769405 [details] [diff] [review]
patch v2
Attachment #769345 - Attachment is obsolete: true
Attachment #769405 - Flags: review?(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
Attachment #769405 - Flags: review?(mcmanus)
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.
Flags: needinfo?(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 :)
Flags: needinfo?(mcmanus)
(Assignee)

Comment 9

4 years ago
Created attachment 770891 [details] [diff] [review]
patch v3

I've added the protocol version as a string in dashboard.
Attachment #769405 - Attachment is obsolete: true
Attachment #770891 - Flags: review?(valentin.gosu)
Attachment #770891 - Flags: review?(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.
Attachment #770891 - Flags: review?(mcmanus) → review-
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)
Attachment #770891 - Flags: review?(valentin.gosu)
(Assignee)

Comment 12

4 years ago
Created attachment 771235 [details] [diff] [review]
Moving Spdy versions enum to nsHtpp.h
Attachment #771235 - Flags: review?(mcmanus)
(Assignee)

Comment 13

4 years ago
Created attachment 771236 [details] [diff] [review]
Expose protocol version
Attachment #770891 - Attachment is obsolete: true
Attachment #771236 - Flags: review?(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
Attachment #771235 - Flags: review?(mcmanus) → review+
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
Attachment #771236 - Flags: review?(mcmanus) → review?(valentin.gosu)
Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

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

Looks good. r=me
Attachment #771236 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 771997 [details] [diff] [review]
Move Spdy versions enum to nsHtpp.h

r=mcmanus
Attachment #771235 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Updated

4 years ago
Keywords: checkin-needed
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
robert, also please actually run the tests on your next try run :) (you just built it)
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.
(Assignee)

Comment 22

4 years ago
sorry, my bad, I've forgotten to update the second patch too. I will do it now.
(Assignee)

Comment 23

4 years ago
Created attachment 772065 [details] [diff] [review]
expose protocol version

r=valentin.gosu
Attachment #771236 - Attachment is obsolete: true
Patch 2 needs rebasing against m-c/inbound. Also, judging by comment 20, a Try run with tests run would also be appreciated :)
Keywords: checkin-needed
(Assignee)

Comment 25

4 years ago
Created attachment 772108 [details] [diff] [review]
expose protocol version

r=valentin.gosu
Attachment #772065 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
I will come soon with a try tun with tests.
(Assignee)

Comment 27

4 years ago
Created attachment 772251 [details] [diff] [review]
Expose protocol version

r=valentin.gosu
Attachment #772108 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=bcc7980e5484
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/79fcaa76d774
https://hg.mozilla.org/integration/mozilla-inbound/rev/346e8cd551af
Assignee: nobody → robertbindar
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79fcaa76d774
https://hg.mozilla.org/mozilla-central/rev/346e8cd551af
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.