streamline h2 stream flow control buffer

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [spdy][necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This was originally posted
https://bugzilla.mozilla.org/show_bug.cgi?id=1247205#c9

It turned out not to solve the problem in that bug, but its a good
refactoring of existing and potentially buggy code. This code has the
advantage of having only one error state (oom) and is lockless.
(Assignee)

Comment 1

3 years ago
Created attachment 8719225 [details] [diff] [review]
streamline h2 stream flow control buffer
Attachment #8719225 - Flags: review?(hurley)
(Assignee)

Updated

3 years ago
Whiteboard: [spdy][necko-active]

Comment 3

3 years ago
Comment on attachment 8719225 [details] [diff] [review]
streamline h2 stream flow control buffer

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

r=me w/changes below

::: netwerk/base/SimpleBuffer.h
@@ +6,5 @@
> +
> +#ifndef SimpleBuffer_h__
> +#define SimpleBuffer_h__
> +
> +#if 0

Regular block comment, please :) (|#if 0| is always a red flag to me)

@@ +8,5 @@
> +#define SimpleBuffer_h__
> +
> +#if 0
> +This class is similar to a nsPipe except it does not have any locking, stores
> +an unbounded amount of data, can only be used on one thread, and has much

So while it's obvious from reading the code that this is only safe to use on one thread, there's no asserting current thread == owning thread. It would be nice to have that (at least in debug builds) so if (when) people start using this somewhere other than h2, errors can be caught sooner.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +263,4 @@
>    if (NS_SUCCEEDED(rv)) {
> +    rv = mSimpleBuffer.Write(buf, *countWritten);
> +    if (NS_FAILED(rv)) {
> +      MOZ_ASSERT (rv == NS_ERROR_OUT_OF_MEMORY);

nit: no space before (
Attachment #8719225 - Flags: review?(hurley) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8720330 [details] [diff] [review]
streamline h2 stream flow control buffer
(Assignee)

Updated

3 years ago
Attachment #8719225 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8720330 - Flags: review+

Comment 7

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