Introduce StreamRegistry::NotifyReaderAbortedBeforeRegistration()

Introduce StreamRegistry::RegisterReaderAbortedStream()

If the reader of the stream is aborted before the stream is registered,
StreamRegistry::RegisterStream() should fail to reduce the memory consumption.
So this change introduces StreamRegistry::RegisterReaderAbortedStream().

We will use this method in this situation:
 - The request from the page is sent to the ServiceWorker.
 - The ServiceWorker creates a Stream and starts uploading the data to the browser process with it.
 - The browser process receives the stream URL in ServiceWorkerHostMsg_FetchEventFinished.
   But it didn't receive StreamHostMsg_StartBuilding yet.
 - The page stops the loading the resource.
  -> Call this method in the browser process.
 - The browser receives StreamHostMsg_StartBuilding and creates a Stream and registers it to the StreamRegistry.
  -> This registration should fail.

BUG=436424
TEST=content_unittests --gtest_filter=StreamTest.*

Committed: https://crrev.com/5f0c294795345faa7a77de46f5e6ed31a6bb2ee3
Cr-Commit-Position: refs/heads/master@{#308025}

[horo, 2014-11-26 07:44:20]

horo@chromium.org changed reviewers:
+ tyoshino@chromium.org

[horo, 2014-11-26 07:44:20]

tyoshino@
Could you please review this?

[tyoshino (SeeGerritForStatus), 2014-11-26 08:06:07]

CL description:

is send -> is sent

the browser receives -> the browser process receives

The page stop the -> The page stops the

[horo, 2014-11-26 08:13:18]

On 2014/11/26 08:06:07, tyoshino wrote:
> CL description:
> 
> is send -> is sent
> 
> the browser receives -> the browser process receives
> 
> The page stop the -> The page stops the

Done. Thank you.

[tyoshino (SeeGerritForStatus), 2014-11-26 09:29:12]

lgtm

https://codereview.chromium.org/759823003/diff/1/content/browser/streams/stre...
File content/browser/streams/stream_unittest.cc (right):

https://codereview.chromium.org/759823003/diff/1/content/browser/streams/stre...
content/browser/streams/stream_unittest.cc:379: ASSERT_EQ(nullptr,
registry_->GetStream(url).get());
please also update NULLs in this file to nullptr

[horo, 2014-11-26 09:54:10]

https://codereview.chromium.org/759823003/diff/1/content/browser/streams/stre...
File content/browser/streams/stream_unittest.cc (right):

https://codereview.chromium.org/759823003/diff/1/content/browser/streams/stre...
content/browser/streams/stream_unittest.cc:379: ASSERT_EQ(nullptr,
registry_->GetStream(url).get());
On 2014/11/26 09:29:12, tyoshino wrote:
> please also update NULLs in this file to nullptr

Done.

[horo, 2014-11-26 09:54:40]

horo@chromium.org changed reviewers:
+ zork@chromium.org

[horo, 2014-11-26 09:54:41]

zork@
Could you please review this?

[horo, 2014-12-02 10:50:40]

zork@
Ping?

[Zachary Kuznia, 2014-12-02 14:50:45]

On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> zork@
> Ping?

This function name is strange, and leaks implementation details.  Since this is
the StreamRegistry, why does it need another class to tell it a stream wasn't
registered?  Is there a reason not to just call it, "NotifyReaderAborted"?

[horo, 2014-12-05 07:12:37]

On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > zork@
> > Ping?
> 
> This function name is strange, and leaks implementation details.  Since this
is
> the StreamRegistry, why does it need another class to tell it a stream wasn't
> registered?  Is there a reason not to just call it, "NotifyReaderAborted"?

If the reader of the stream is killed after the stream is registered, we can
stop adding the data to the stream by deleting the StreamHandle.
But the reader of the stream may be killed before the stream is registered.
At this situation we can't stop adding the data because the reader didn't get
the StreamHandle yet.

To support this, we need NotifyReaderAbortedBeforeRegistration().

[Zachary Kuznia, 2014-12-05 15:18:34]

On 2014/12/05 07:12:37, horo wrote:
> On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > zork@
> > > Ping?
> > 
> > This function name is strange, and leaks implementation details.  Since this
> is
> > the StreamRegistry, why does it need another class to tell it a stream
wasn't
> > registered?  Is there a reason not to just call it, "NotifyReaderAborted"?
> 
> If the reader of the stream is killed after the stream is registered, we can
> stop adding the data to the stream by deleting the StreamHandle.
> But the reader of the stream may be killed before the stream is registered.
> At this situation we can't stop adding the data because the reader didn't get
> the StreamHandle yet.
> 
> To support this, we need NotifyReaderAbortedBeforeRegistration().

Do you have an example of a caller of this function?

[horo, 2014-12-05 20:32:40]

On 2014/12/05 15:18:34, Zachary Kuznia wrote:
> On 2014/12/05 07:12:37, horo wrote:
> > On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > > zork@
> > > > Ping?
> > > 
> > > This function name is strange, and leaks implementation details.  Since
this
> > is
> > > the StreamRegistry, why does it need another class to tell it a stream
> wasn't
> > > registered?  Is there a reason not to just call it, "NotifyReaderAborted"?
> > 
> > If the reader of the stream is killed after the stream is registered, we can
> > stop adding the data to the stream by deleting the StreamHandle.
> > But the reader of the stream may be killed before the stream is registered.
> > At this situation we can't stop adding the data because the reader didn't
get
> > the StreamHandle yet.
> > 
> > To support this, we need NotifyReaderAbortedBeforeRegistration().
> 
> Do you have an example of a caller of this function?

Yes.
ServiceWorkerURLRequestJob will use this method.
See: https://codereview.chromium.org/759203002/
https://codereview.chromium.org/759203002/

[Zachary Kuznia, 2014-12-05 21:09:02]

On 2014/12/05 20:32:40, horo(ooo Dec.8) wrote:
> On 2014/12/05 15:18:34, Zachary Kuznia wrote:
> > On 2014/12/05 07:12:37, horo wrote:
> > > On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > > > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > > > zork@
> > > > > Ping?
> > > > 
> > > > This function name is strange, and leaks implementation details.  Since
> this
> > > is
> > > > the StreamRegistry, why does it need another class to tell it a stream
> > wasn't
> > > > registered?  Is there a reason not to just call it,
"NotifyReaderAborted"?
> > > 
> > > If the reader of the stream is killed after the stream is registered, we
can
> > > stop adding the data to the stream by deleting the StreamHandle.
> > > But the reader of the stream may be killed before the stream is
registered.
> > > At this situation we can't stop adding the data because the reader didn't
> get
> > > the StreamHandle yet.
> > > 
> > > To support this, we need NotifyReaderAbortedBeforeRegistration().
> > 
> > Do you have an example of a caller of this function?
> 
> Yes.
> ServiceWorkerURLRequestJob will use this method.
> See: https://codereview.chromium.org/759203002/
> https://codereview.chromium.org/759203002/

This function name explains why you're calling the function, instead of what the
function is doing.  In your calling code, it's important to know that the reader
was aborted before the stream was registered.  What you're asking the Registry
to do is to abort the stream when it's registered; the name of the function
should reflect this.

[horo, 2014-12-09 03:19:59]

On 2014/12/05 21:09:02, Zachary Kuznia wrote:
> On 2014/12/05 20:32:40, horo(ooo Dec.8) wrote:
> > On 2014/12/05 15:18:34, Zachary Kuznia wrote:
> > > On 2014/12/05 07:12:37, horo wrote:
> > > > On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > > > > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > > > > zork@
> > > > > > Ping?
> > > > > 
> > > > > This function name is strange, and leaks implementation details. 
Since
> > this
> > > > is
> > > > > the StreamRegistry, why does it need another class to tell it a stream
> > > wasn't
> > > > > registered?  Is there a reason not to just call it,
> "NotifyReaderAborted"?
> > > > 
> > > > If the reader of the stream is killed after the stream is registered, we
> can
> > > > stop adding the data to the stream by deleting the StreamHandle.
> > > > But the reader of the stream may be killed before the stream is
> registered.
> > > > At this situation we can't stop adding the data because the reader
didn't
> > get
> > > > the StreamHandle yet.
> > > > 
> > > > To support this, we need NotifyReaderAbortedBeforeRegistration().
> > > 
> > > Do you have an example of a caller of this function?
> > 
> > Yes.
> > ServiceWorkerURLRequestJob will use this method.
> > See: https://codereview.chromium.org/759203002/
> > https://codereview.chromium.org/759203002/
> 
> This function name explains why you're calling the function, instead of what
the
> function is doing.  In your calling code, it's important to know that the
reader
> was aborted before the stream was registered.  What you're asking the Registry
> to do is to abort the stream when it's registered; the name of the function
> should reflect this.

I got it.
Renamed to RegisterReaderAbortedStream.

[Zachary Kuznia, 2014-12-10 23:17:36]

On 2014/12/09 03:19:59, horo wrote:
> On 2014/12/05 21:09:02, Zachary Kuznia wrote:
> > On 2014/12/05 20:32:40, horo(ooo Dec.8) wrote:
> > > On 2014/12/05 15:18:34, Zachary Kuznia wrote:
> > > > On 2014/12/05 07:12:37, horo wrote:
> > > > > On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > > > > > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > > > > > zork@
> > > > > > > Ping?
> > > > > > 
> > > > > > This function name is strange, and leaks implementation details. 
> Since
> > > this
> > > > > is
> > > > > > the StreamRegistry, why does it need another class to tell it a
stream
> > > > wasn't
> > > > > > registered?  Is there a reason not to just call it,
> > "NotifyReaderAborted"?
> > > > > 
> > > > > If the reader of the stream is killed after the stream is registered,
we
> > can
> > > > > stop adding the data to the stream by deleting the StreamHandle.
> > > > > But the reader of the stream may be killed before the stream is
> > registered.
> > > > > At this situation we can't stop adding the data because the reader
> didn't
> > > get
> > > > > the StreamHandle yet.
> > > > > 
> > > > > To support this, we need NotifyReaderAbortedBeforeRegistration().
> > > > 
> > > > Do you have an example of a caller of this function?
> > > 
> > > Yes.
> > > ServiceWorkerURLRequestJob will use this method.
> > > See: https://codereview.chromium.org/759203002/
> > > https://codereview.chromium.org/759203002/
> > 
> > This function name explains why you're calling the function, instead of what
> the
> > function is doing.  In your calling code, it's important to know that the
> reader
> > was aborted before the stream was registered.  What you're asking the
Registry
> > to do is to abort the stream when it's registered; the name of the function
> > should reflect this.
> 
> I got it.
> Renamed to RegisterReaderAbortedStream.

This name still reflects why you're doing it, instead of what is being done.  It
should be something more along the lines of:
CancelStream, or AbortPendingStream.

[horo, 2014-12-11 01:01:19]

On 2014/12/10 23:17:36, Zachary Kuznia wrote:
> On 2014/12/09 03:19:59, horo wrote:
> > On 2014/12/05 21:09:02, Zachary Kuznia wrote:
> > > On 2014/12/05 20:32:40, horo(ooo Dec.8) wrote:
> > > > On 2014/12/05 15:18:34, Zachary Kuznia wrote:
> > > > > On 2014/12/05 07:12:37, horo wrote:
> > > > > > On 2014/12/02 14:50:45, Zachary Kuznia wrote:
> > > > > > > On 2014/12/02 10:50:40, horo(ooo -Dec.3) wrote:
> > > > > > > > zork@
> > > > > > > > Ping?
> > > > > > > 
> > > > > > > This function name is strange, and leaks implementation details. 
> > Since
> > > > this
> > > > > > is
> > > > > > > the StreamRegistry, why does it need another class to tell it a
> stream
> > > > > wasn't
> > > > > > > registered?  Is there a reason not to just call it,
> > > "NotifyReaderAborted"?
> > > > > > 
> > > > > > If the reader of the stream is killed after the stream is
registered,
> we
> > > can
> > > > > > stop adding the data to the stream by deleting the StreamHandle.
> > > > > > But the reader of the stream may be killed before the stream is
> > > registered.
> > > > > > At this situation we can't stop adding the data because the reader
> > didn't
> > > > get
> > > > > > the StreamHandle yet.
> > > > > > 
> > > > > > To support this, we need NotifyReaderAbortedBeforeRegistration().
> > > > > 
> > > > > Do you have an example of a caller of this function?
> > > > 
> > > > Yes.
> > > > ServiceWorkerURLRequestJob will use this method.
> > > > See: https://codereview.chromium.org/759203002/
> > > > https://codereview.chromium.org/759203002/
> > > 
> > > This function name explains why you're calling the function, instead of
what
> > the
> > > function is doing.  In your calling code, it's important to know that the
> > reader
> > > was aborted before the stream was registered.  What you're asking the
> Registry
> > > to do is to abort the stream when it's registered; the name of the
function
> > > should reflect this.
> > 
> > I got it.
> > Renamed to RegisterReaderAbortedStream.
> 
> This name still reflects why you're doing it, instead of what is being done. 
It
> should be something more along the lines of:
> CancelStream, or AbortPendingStream.

I changed to AbortPendingStream.
Thank you.

[Zachary Kuznia, 2014-12-11 18:32:57]

lgtm

[horo, 2014-12-11 23:59:32]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-12-12 00:01:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759823003/60001

[commit-bot: I haz the power, 2014-12-12 02:02:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 02:03:31]

Patchset 4 (id:??) landed as
https://crrev.com/5f0c294795345faa7a77de46f5e6ed31a6bb2ee3
Cr-Commit-Position: refs/heads/master@{#308025}