Closed Bug 1248203 Opened 8 years ago Closed 8 years ago

streamline h2 stream flow control buffer

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8719225 - Flags: review?(hurley)
Whiteboard: [spdy][necko-active]
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+
Attachment #8719225 - Attachment is obsolete: true
Attachment #8720330 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1fff1b2ac665
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: