Closed Bug 1425097 Opened 2 years ago Closed 2 years ago

We do not need to shutdown a http2session is a http transaction receives 425

Categories

(Core :: Networking: HTTP, enhancement, P2)

59 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

Currently we call Http2Session::DontReuse if we want restart a transaction after receiving 425. This will eventually close the session. We could only reset the stream.

(We still need to call DontReuse if h1 connection is used)
Attached patch bug_1425097_v1.patch (obsolete) — Splinter Review
Attachment #8942675 - Flags: review?(mcmanus)
there are other cases when we do not need to close h2 connection just the stream, but this bug deals only with 425.
Attachment #8942675 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/070b636b242b
We do not to close h2 connection in case one stream gets 425. r=mcmanus
Keywords: checkin-needed
Comment on attachment 8942675 [details] [diff] [review]
bug_1425097_v1.patch

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1676,5 @@
>              LOG(("Too Early."));
>              if ((mEarlyDataDisposition == EARLY_425) && !mDoNotTryEarlyData) {
>                  mDoNotTryEarlyData = true;
>                  mForceRestart = true; // force restart has built in loop protection
> +                if (mConnection->Version() = HTTP_VERSION_2) {

I think this was supposed to be "==", not "=", right?

This actually triggers a clang build error for me locally ("nsHttpTransaction.cpp:1668:44: error: expression is not assignable"), though I'm using bleeding-edge clang.
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 8942675 [details] [diff] [review]
> bug_1425097_v1.patch
> 
> Review of attachment 8942675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1676,5 @@
> >              LOG(("Too Early."));
> >              if ((mEarlyDataDisposition == EARLY_425) && !mDoNotTryEarlyData) {
> >                  mDoNotTryEarlyData = true;
> >                  mForceRestart = true; // force restart has built in loop protection
> > +                if (mConnection->Version() = HTTP_VERSION_2) {
> 
> I think this was supposed to be "==", not "=", right?
> 
> This actually triggers a clang build error for me locally
> ("nsHttpTransaction.cpp:1668:44: error: expression is not assignable"),
> though I'm using bleeding-edge clang.

sorry it should be ==
Flags: needinfo?(dd.mozilla)
Backed out for bustage at netwerk/protocol/http/nsHttpTransaction.cpp:1668: lvalue required as left operand of assignment:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7a7bc878bcf5424c36ae080789473e51351a74

Push with bustage (previous pushes had bustage from a different patch): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=aa97f8900daef878107eb17f1afd802a88605544&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=156640577&repo=mozilla-inbound

[task 2018-01-16T18:08:40.650Z] 18:08:40     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/http/Unified_cpp_protocol_http2.cpp:74:0:
[task 2018-01-16T18:08:40.650Z] 18:08:40     INFO -  /builds/worker/workspace/build/src/netwerk/protocol/http/nsHttpTransaction.cpp: In member function 'nsresult mozilla::net::nsHttpTransaction::HandleContentStart()':
[task 2018-01-16T18:08:40.650Z] 18:08:40     INFO -  /builds/worker/workspace/build/src/netwerk/protocol/http/nsHttpTransaction.cpp:1668:46: error: lvalue required as left operand of assignment
[task 2018-01-16T18:08:40.650Z] 18:08:40     INFO -                   if (mConnection->Version() = HTTP_VERSION_2) {
[task 2018-01-16T18:08:40.651Z] 18:08:40     INFO -                                                ^~~~~~~~~~~~~~
[task 2018-01-16T18:08:40.651Z] 18:08:40     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1040: recipe for target 'Unified_cpp_protocol_http2.o' failed
[task 2018-01-16T18:08:40.651Z] 18:08:40     INFO -  gmake[4]: *** [Unified_cpp_protocol_http2.o] Error 1
Flags: needinfo?(dd.mozilla)
Attachment #8942675 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8943273 - Flags: review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a74cfcfe1a
We do not to close h2 connection in case one stream gets 425. r=mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1a74cfcfe1a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.