Closed Bug 1354233 Opened 2 years ago Closed 2 years ago

We append h2 stream to m0RTTStreams to early

Categories

(Core :: Networking: HTTP, defect)

54 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

Place where we put a h2 stream into m0RTTStreams is too early. At the point when we append it  streamId is 0.

    if (!m0RTTStreams.Contains(stream->StreamID())) {
      m0RTTStreams.AppendElement(stream->StreamID());
    }
Blocks: 1352274
Version: 53 Branch → 54 Branch
Attached patch bug_1354233_v1.patch (obsolete) — Splinter Review
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Attachment #8855693 - Flags: review?(hurley)
Whiteboard: [necko-active]
Comment on attachment 8855693 [details] [diff] [review]
bug_1354233_v1.patch

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

drive by review.. thanks. maybe add a log too when appending it. and we should try and hit 54 before it goes to beta.
Attachment #8855693 - Flags: review+
log added.
Attachment #8855693 - Attachment is obsolete: true
Attachment #8855693 - Flags: review?(hurley)
Attachment #8855832 - Flags: review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69022c2227f
Add stream to m0RTTStreams after streamId is set. r=mcmanus
Comment on attachment 8855832 [details] [diff] [review]
bug_1354233_v1.patch

Approval Request Comment
[Feature/Bug causing the regression]: TLS1.3 early-data for Http2 - bug 1322373
[User impact if declined]:If early-data is not accepted by the server and server switch from h2 to h1 (this is not happening very often) we will have a strange behavior, probably needs refresh to load the affected page.
In all other cases, early-data accepted or not, it can block the load in some cases.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: I have verified it.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]: no, it moves 2 lines of code a couple of lines down in the same function and we hold the reference so it will not break.
[Why is the change risky/not risky?]:no
[String changes made/needed]:none
Attachment #8855832 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d69022c2227f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8855832 [details] [diff] [review]
bug_1354233_v1.patch

Fix an issue related to TLS1.3 early-data for Http2. Aurora54+.
Attachment #8855832 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.