Do not close SpdyConnectTransaction in TLSFilterTransaction

VERIFIED FIXED in Firefox -esr68

Status

()

defect
P1
normal
VERIFIED FIXED
2 months ago
Last month

People

(Reporter: dragana, Assigned: dragana)

Tracking

(Blocks 1 bug, Regression)

68 Branch
mozilla69
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr6868+ fixed, firefox68+ fixed, firefox69 fixed)

Details

(Whiteboard: [necko-triaged][secure-proxy-mvp])

Attachments

(1 attachment)

This is a regression from bug 1484947.

Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/7d95f38f8dad
Do not close SpdyConnectTransaction in TLSFilterTransaction. r=mayhemer
Blocks: 1545411
Whiteboard: [necko-triaged][secure-proxy-mvp]
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9076167 [details]
Do not close SpdyConnectTransaction in TLSFilterTransaction. r=mayhemer

Beta/Release Uplift Approval Request

  • User impact if declined: Https proxy that uses http2 will not work properly. The connection will fill slower than it is.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a (recent) regression. It was introduced by bug 1484947. Before the bug landed this code was running fine for years. This patch just removes remaining part of the code introduced by bug 1484947 (The other part was removed in bug 1563695). There has been no changes to this code that could rely on the code introduced in bug 1484947.
  • String changes made/needed:
Attachment #9076167 - Flags: approval-mozilla-beta?
Blocks: 1484947

You should have asked release approval.

Comment on attachment 9076167 [details]
Do not close SpdyConnectTransaction in TLSFilterTransaction. r=mayhemer

Beta/Release Uplift Approval Request

  • User impact if declined: Https proxy that uses http2 will not work properly. The connection will fill slower than it is.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a (recent) regression. It was introduced by bug 1484947. Before the bug landed this code was running fine for years. This patch just removes remaining part of the code introduced by bug 1484947 (The other part was removed in bug 1563695). There has been no changes to this code that could rely on the code introduced in bug 1484947.
  • String changes made/needed:
Attachment #9076167 - Flags: approval-mozilla-release?

Comment on attachment 9076167 [details]
Do not close SpdyConnectTransaction in TLSFilterTransaction. r=mayhemer

this is already in 69.

Attachment #9076167 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

I will mark this issue qe-verify- as per comment 4 and comment 6.

Flags: qe-verify-

I verified this on 68 using the manual add-on, confirmed that visiting https://expired.badssl.com/ generates the appropriate certificate error message.

(In reply to Rebecca Billings [:rbillings] from comment #9)

I verified this on 68 using the manual add-on, confirmed that visiting https://expired.badssl.com/ generates the appropriate certificate error message.

I don't think this has anything to do with this bug.

Please add repro steps with expected results. I was going from comment 1, and if that doesn't relate then this needs more info.

Flags: needinfo?(odvarko)
Flags: needinfo?(honzab.moz)

There is no specific set of steps to verify this bug. This bug is complementary to bug 1563538, you can verify them both using the same set of steps. General browsing on reddit, facebook, youtube (feel free to any heavier sites to the list) at the same time should lead to no interruptions, broken or infinite time taking loads.

Flags: needinfo?(honzab.moz)

Testing with 3+ tabs, I am getting consistent [1 or more per view] lags watching youtube videos. The pauses last from a few seconds up to maybe 30s. If there is additional detail that I can add from logs or the console that would be helpful, let me know.

I was using yesterday's build, will update with today's build and re-test.

While testing there was a slightly noticeable load time when skipping through different videos if letting the videos run normally, no issues were noticeable.

Normal browsing didn't suffer much, Facebook, Reddit, Twitter and Youtube working as intended without any kind of hangs or infinite loading times happening, but slower loading of the pages was noticeable in some cases compared to having the proxy turned off.

I tested multiple tabs and browsed through all of the sites above, running videos. I was not getting any consistent lags or delays in video playback or page load.

Status: RESOLVED → VERIFIED

Comment on attachment 9076167 [details]
Do not close SpdyConnectTransaction in TLSFilterTransaction. r=mayhemer

fix for http2 proxies, needed for the secure-proxy project, approved for 68.0.1 and 68.1esr

Attachment #9076167 - Flags: approval-mozilla-release?
Attachment #9076167 - Flags: approval-mozilla-release+
Attachment #9076167 - Flags: approval-mozilla-esr68+

clearing NI for the other Honza.

Flags: needinfo?(odvarko)
Regressed by: 1484947

Per discussion with jcristau, we're uplifting this to 68.0.1esr also to maintain parity with the non-ESR 68.0.1 release and hopefully avoid some confusion.

You need to log in before you can comment on or make changes to this bug.