Create TLSTransportLayer - for adding a secondary TLS session when HTTPS proxy is used
Categories
(Core :: Networking: HTTP, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox105 | --- | fixed |
People
(Reporter: dragana, Assigned: kershaw)
References
(Blocks 1 open bug)
Details
(Keywords: perf-alert, Whiteboard: [necko-triaged])
Attachments
(5 files)
Current state:
TLSFilterTransaction is used for creating the secondary TLS session. The TLS handshake is driven by timers that nudge TLSFilterTransaction in regular intervals. The timers are not necessary as TLS handshakes have well-defined triggers, i.e. writing ClientHello, reading ServerHello, certificate authentication, etc. TLSFilterTransaction is situated between nsHttpConnection and nsHttpTransaction, layering it between nsHttpConnection and nsSocketTransport(or HTTP/2 stream) is better.
Proposal:
Create TLSTransportLayer and helper classes TLSTransportLayerOutputStream and TLSTransportLayerInputStream.
TLSTransportLayer will provide a secondary TLS layer. It will be added as a layer between nsHttpConnection and nsSocketTransport or Http2StreamConnection.
TLSTransportLayer needs to implement:
- nsISocketTransport
- nsIInputStreamCallback
- nsIOutputStreamCallback
TLSTransportLayerOutputStream and TLSTransportLayerInputStream need to implement:
- nsIAsyncInputStream
- nsIAsyncOutputStream
Note: we need to separate classes because the interfaces have a collision.
Functionalities:
TLSTransportLayer contains a TLS layer. The layer will read data directly form the underlying stream(a OS socket or Http2Stream) and decrypt the data. The encrypted data will be written into a buffer and from the buffer into the underlying socket.
Helper classes that implement nsIAsyncInputStream and nsIAsyncOutputStream are used for triggering writes and reads. AsyncWait-s of the nsIAsyncInputStream and nsIAsyncOutputStream interface are called when the consumer wants to read/write data. When the underlying stream, which can be a OS socket or Http2Stream, can accept more data fro writing or has data o be read calls OnInputStreamReady/OnOutputStreamReady of the nsIInputStreamCallback and nsIOutputStreamCallback interface.
TLSTransportLayer, TLSTransportLayerOutputStream and TLSTransportLayerInputStream will propagate these function calls in both direction, to the nsSocketTransport/Http2StreamConnection and to nsHttpConnection. The following behavior is different:
- When the buffer with encrypted data is not empty, call AsyncWait on the nsIAsyncOutputStream for the underlying nsSocketTransport or Http2StreamConnection (this will poll the OS socket for writing).
- When nsHttpConnection call AsyncWait on the nsIAsyncInputStream:
- If TLS layer has unread data, dispatch nsHttpConnection::OnInputStreamReady
- Otherwise, call AsyncWait on the nsIAsyncInputStream for the underlying socket transport or Http2StreamConnection.
- TLSTransportLayer::OnOutputStreamReady needs to writes that from the buffer with encrypted data into the underlying stream (i.e. the OS socket or Http2Stream).
To investigate: We may be able to remove the buffer that holds encrypted data, at least for HTTP/2 proxy and maybe also for HTTP/1.1 proxy.
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Comment 1•2 years ago
|
||
Note that this patch only works for Http/1.1 case.
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 2•2 years ago
|
||
| Comment hidden (off-topic) |
|
||
Comment 4•2 years ago
|
||
Sorry, there was a problem with the detection of inactive users. I'm reverting the change.
|
|
Assignee | |
Comment 5•2 years ago
|
||
|
|
Assignee | |
Comment 6•2 years ago
|
||
|
|
Assignee | |
Comment 7•2 years ago
|
||
|
|
Assignee | |
Comment 8•2 years ago
|
||
This patch fixes two issues found on try:
- Make the compressor output errors based on the input headers
- Make sure the weak ptr is always released on socket thread
Pushed by kjang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a24a888922f1 Introduce TLSTransportLayer, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/ce4a38b31322 Support plain http over Http/2 proxy, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/5944e15ddfc7 Improve the closing of TLSTransportLayer, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/813b707d8471 Remove TLSFilterTransaction, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/2003686cc6b8 Fix intermittent test failure, r=necko-reviewers,dragana
|
||
Comment 10•2 years ago
|
||
Backed out for causing bp-hybrid bustages on Http2StreamTunnel.
Failure log from OS X Cross Compiled plain bp-hybrid bustage
Failure log from Windows 2012 x64 debug bp-hybrid bustage
[task 2022-08-04T12:32:59.569Z] 12:32:59 INFO - gmake[4]: Entering directory '/builds/worker/workspace/obj-build/netwerk/protocol/http'
[task 2022-08-04T12:32:59.572Z] 12:32:59 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/MacOSX11.0.sdk -mmacosx-version-min=10.12 -stdlib=libc++ -std=gnu++17 --target=x86_64-apple-darwin -o Http2StreamTunnel.o -c -fvisibility=hidden -fvisibility-inlines-hidden -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DHAS_CONNECTX -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/netwerk/protocol/http -I/builds/worker/workspace/obj-build/netwerk/protocol/http -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/cookie -I/builds/worker/checkouts/gecko/netwerk/dns -I/builds/worker/checkouts/gecko/netwerk/ipc -I/builds/worker/checkouts/gecko/netwerk/socket/neqo_glue -I/builds/worker/checkouts/gecko/netwerk/url-classifier -I/builds/worker/checkouts/gecko/extensions/auth -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wno-ambiguous-reversed-operator -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-enum-float-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wno-error=deprecated-volatile -Wcomma -Wimplicit-fallthrough -Wduplicate-method-arg -Wduplicate-method-match -Wmissing-method-return-type -Wobjc-signed-char-bool -Wsemicolon-before-method-body -Wsuper-class-method-mismatch -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=free-nonheap-object -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wthread-safety -Wno-unknown-warning-option -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -ffp-contract=off -MD -MP -MF .deps/Http2StreamTunnel.o.pp /builds/worker/checkouts/gecko/netwerk/protocol/http/Http2StreamTunnel.cpp
[task 2022-08-04T12:32:59.574Z] 12:32:59 ERROR - /builds/worker/checkouts/gecko/netwerk/protocol/http/Http2StreamTunnel.cpp:352:35: error: use of undeclared identifier 'do_QueryObject'
[task 2022-08-04T12:32:59.575Z] 12:32:59 INFO - RefPtr<nsHttpConnection> conn = do_QueryObject(aCallback);
[task 2022-08-04T12:32:59.575Z] 12:32:59 INFO - ^
[task 2022-08-04T12:32:59.576Z] 12:32:59 INFO - 1 error generated.
[task 2022-08-04T12:32:59.576Z] 12:32:59 ERROR - gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:668: Http2StreamTunnel.o] Error 1
[task 2022-08-04T12:32:59.577Z] 12:32:59 INFO - gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/netwerk/protocol/http'
[task 2022-08-04T12:32:59.577Z] 12:32:59 INFO - gmake[4]: *** Waiting for unfinished jobs....
[task 2022-08-04T12:32:59.578Z] 12:32:59 INFO - gmake[4]: Entering directory '/builds/worker/workspace/obj-build/third_party/libwebrtc/common_audio/common_audio_gn'
|
|
Assignee | |
Comment 11•2 years ago
|
||
|
||
Comment 12•2 years ago
|
||
Pushed by kjang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8f0ca2710ee1 Introduce TLSTransportLayer, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/21b272c5afe5 Support plain http over Http/2 proxy, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/e92fb7d8e984 Improve the closing of TLSTransportLayer, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/7ac049634840 Remove TLSFilterTransaction, r=necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/efd9f190d506 Fix intermittent test failure, r=necko-reviewers,dragana
|
||
Comment 13•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/8f0ca2710ee1
https://hg.mozilla.org/mozilla-central/rev/21b272c5afe5
https://hg.mozilla.org/mozilla-central/rev/e92fb7d8e984
https://hg.mozilla.org/mozilla-central/rev/7ac049634840
https://hg.mozilla.org/mozilla-central/rev/efd9f190d506
|
||
Comment 14•2 years ago
|
||
(In reply to Pulsebot from comment #12)
Pushed by kjang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f0ca2710ee1
Introduce TLSTransportLayer, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/21b272c5afe5
Support plain http over Http/2 proxy, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/e92fb7d8e984
Improve the closing of TLSTransportLayer, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/7ac049634840
Remove TLSFilterTransaction, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/efd9f190d506
Fix intermittent test failure, r=necko-reviewers,dragana
== Change summary for alert #35040 (as of Mon, 08 Aug 2022 00:54:55 GMT) ==
Regressions:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 198% | wikia loadtime | windows10-64-shippable-qr | cold fission webrender | 2,401.17 -> 7,157.71 |
| 153% | imgur loadtime | macosx1015-64-shippable-qr | fission warm webrender | 453.88 -> 1,146.96 |
| 131% | outlook LastVisualChange | windows10-64-shippable-qr | fission warm webrender | 2,611.25 -> 6,039.42 |
| 116% | imgur loadtime | windows10-64-shippable-qr | fission warm webrender | 530.38 -> 1,146.12 |
| 100% | imgur LastVisualChange | windows10-64-shippable-qr | fission warm webrender | 3,386.00 -> 6,780.50 |
| 72% | outlook LastVisualChange | windows10-64-shippable-qr | cold fission webrender | 4,044.75 -> 6,947.67 |
| 57% | outlook fcp | macosx1015-64-shippable-qr | fission warm webrender | 289.06 -> 454.50 |
| 55% | outlook FirstVisualChange | macosx1015-64-shippable-qr | fission warm webrender | 310.00 -> 480.00 |
| 48% | outlook fcp | windows10-64-shippable-qr | fission warm webrender | 211.29 -> 313.04 |
| 43% | outlook FirstVisualChange | windows10-64-shippable-qr | fission warm webrender | 234.00 -> 334.00 |
| ... | ... | ... | ... | ... |
| 38% | imgur loadtime | macosx1015-64-shippable-qr | cold fission webrender | 1,392.00 -> 1,923.67 |
| 29% | imgur ContentfulSpeedIndex | windows10-64-shippable-qr | fission warm webrender | 507.67 -> 653.00 |
| 24% | imgur loadtime | windows10-64-shippable-qr | cold fission webrender | 1,772.67 -> 2,205.04 |
| 15% | imgur PerceptualSpeedIndex | macosx1015-64-shippable-qr | fission warm webrender | 365.08 -> 419.58 |
| 11% | imgur LastVisualChange | windows10-64-shippable-qr | cold fission webrender | 6,955.75 -> 7,716.83 |
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 77% | outlook SpeedIndex | windows10-64-shippable-qr | cold fission webrender | 2,292.58 -> 528.58 |
| 76% | outlook SpeedIndex | macosx1015-64-shippable-qr | cold fission webrender | 2,387.67 -> 581.58 |
| 63% | outlook ContentfulSpeedIndex | macosx1015-64-shippable-qr | cold fission webrender | 2,055.75 -> 752.58 |
| 57% | outlook SpeedIndex | windows10-64-shippable-qr | fission warm webrender | 1,082.75 -> 469.75 |
| 47% | outlook PerceptualSpeedIndex | windows10-64-shippable-qr | cold fission webrender | 836.58 -> 443.92 |
| ... | ... | ... | ... | ... |
| 3% | google-docs ContentfulSpeedIndex | macosx1015-64-shippable-qr | cold fission webrender | 1,412.92 -> 1,367.25 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35040
|
|
||
Updated•2 years ago
|
|
||
Comment 15•2 years ago
|
||
== Change summary for alert #35125 (as of Mon, 15 Aug 2022 23:40:54 GMT) ==
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 3% | JS | linux1804-64-shippable-qr | fission tp6 | 178,544,273.67 -> 173,155,069.66 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35125
Description
•