Improve connection management

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
1) if a connection is read, i.e. socket is connected, and we can attach a pending transaction or a NullTransaction, for https connections, it can happen that the connection is put into idle queue even though it could take a NullTransaction to perform tls. This can happen when the transaction pending queue is not empty but all transaction in the queue are content-blocked.

2) If transaction takes a speculatively open halfOpenSocket, the halfOpenSocket will not backup socket as in other cases.

3) If a halfOpenSocket is opened for a transaction, but the transaction takes another connection, the halfOpenSocket cannot be claimed by another transaction.
(Assignee)

Updated

2 years ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
(Assignee)

Updated

2 years ago
Depends on: 1341572
(Assignee)

Comment 1

2 years ago
Created attachment 8844421 [details] [diff] [review]
bug_1344171_v1.patch
Attachment #8844421 - Flags: review?(mcmanus)
Comment on attachment 8844421 [details] [diff] [review]
bug_1344171_v1.patch

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

oddly, this seems simpler. thanks!
Attachment #8844421 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8849068 [details] [diff] [review]
bug_1344171_v1.patch

Rebased.
Attachment #8844421 - Attachment is obsolete: true
Attachment #8849068 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 4

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0db7810a84
Improve connection management. r=mcmanus
Keywords: checkin-needed
Backed out for bustage (unused variable 'claimed'):

https://hg.mozilla.org/integration/mozilla-inbound/rev/0afa705631ce000b3eef48deef5d618b510a2fc0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8646978d35260c7a796f776d5d03b9d62828bbaf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85117771&repo=mozilla-inbound

[task 2017-03-20T19:25:59.855081Z] 19:25:59     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/netwerk/protocol/http/Unified_cpp_protocol_http1.cpp:110:0:
[task 2017-03-20T19:25:59.855394Z] 19:25:59     INFO -  /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpConnectionMgr.cpp: In member function 'nsresult mozilla::net::nsHttpConnectionMgr::CreateTransport(mozilla::net::nsHttpConnectionMgr::nsConnectionEntry*, mozilla::net::nsAHttpTransaction*, uint32_t, bool, bool, bool, mozilla::net::nsHttpConnectionMgr::PendingTransactionInfo*)':
[task 2017-03-20T19:25:59.856385Z] 19:25:59     INFO -  /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpConnectionMgr.cpp:1826:14: error: unused variable 'claimed' [-Werror=unused-variable]
[task 2017-03-20T19:25:59.856581Z] 19:25:59     INFO -           bool claimed = sock->Claim();
[task 2017-03-20T19:25:59.856760Z] 19:25:59     INFO -                ^
[task 2017-03-20T19:25:59.856949Z] 19:25:59     INFO -  cc1plus: all warnings being treated as errors
[task 2017-03-20T19:25:59.857182Z] 19:25:59     INFO -  /home/worker/workspace/build/src/config/rules.mk:1016: recipe for target 'Unified_cpp_protocol_http1.o' failed
[task 2017-03-20T19:25:59.857359Z] 19:25:59     INFO -  gmake[5]: *** [Unified_cpp_protocol_http1.o] Error 1
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 6

2 years ago
Created attachment 8849623 [details] [diff] [review]
bug_1344171_v1.patch
Attachment #8849068 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8849623 - Flags: review+

Comment 7

2 years ago
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c040782ca691
Improve connection management. r=mcmanus
This appears to have caused a frequent but intermittent xpcshell crash on Win7VM debug: https://treeherder.mozilla.org/logviewer.html#?job_id=85455754&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4c45a7779491
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 11

2 years ago
Created attachment 8852081 [details] [diff] [review]
bug_1344171_v1.patch
Attachment #8849623 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8852081 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d11fb28f9b9c
Improve connection management. r=mcmanus
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/552267771708 for pretty much the same Win7 test_altsvc.js crashes, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=87151902&repo=mozilla-inbound (the rest of the failures after test_altsvc.js are unrelated, just other bustage from another busted patch in the same push).
(Assignee)

Comment 14

2 years ago
(In reply to Phil Ringnalda (:philor) from comment #13)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/552267771708 for
> pretty much the same Win7 test_altsvc.js crashes, e.g.
> https://treeherder.mozilla.org/logviewer.html#?job_id=87151902&repo=mozilla-
> inbound (the rest of the failures after test_altsvc.js are unrelated, just
> other bustage from another busted patch in the same push).

Thank you. So sorry this was my mistake, I am on IETF and a bit distracted. sorry!
(Assignee)

Comment 17

2 years ago
Created attachment 8853065 [details] [diff] [review]
bug_1344171_v1.patch
Attachment #8852081 - Attachment is obsolete: true
Attachment #8853065 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/364acbf6a069
Improve connection management. r=mcmanus
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/364acbf6a069
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.