Finalize a throttled FileReader in stages.

Finalize a throttled FileReader in stages.

File read operations are throttled, a controller keeping track of a
set of running readers, and those waiting for a turn. In the case of a
running FileReader just completing its operation and an onload* event
listener issuing a new read operation over the same reader, the
throttling controller must correctly handle the renewal of that
FileReader as a running (or now-pending) reader.

This includes signalling that there's pending activity starting (and
later on, completing) on the FileReader itself, something that wasn't
correctly in this nested case.

To address, a FileReader will now remove itself from its throttling
controller in two stages:

  - once the readyState transitions from LOADING, the FileReader is
    removed as a running reader.

  - once the FileReader has dispatched all the required events, the
    FileReader itself signals that the pending activity has completed.

In combination, this balances the reference counting on the underlying
object.

R=kinuko@chromium.org
BUG=345608
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167706

[haraken, 2014-02-21 10:01:34]

kinuko-san: would you take a look?

[sof, 2014-02-21 10:34:35]

Please have a look when you get the chance.

Discovered while moving the File API over to Oilpan. No new tests added here,
but if you run fast/files with just the pushReader() ASSERT() added, you'll see
the problem trigger (with current master.)

[kinuko, 2014-02-21 15:25:48]

(+cc horo)

Is it possible to add a test for this?

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:79: class RemovedReader {
nit: could we rename RemoveReader to ReaderRemover (per our naming convention)

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:81: RemovedReader(FileReader* reader, bool
wasRunning)
This is internal impl so it's not strongly required but using enum is preferred
over bool parameter

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:289:
throttlingController()->removeReader(this);
Is the intention here is to execute RemoveReader's dtor after firing events?  If
so don't we need to receive the return value?

[sof, 2014-02-21 15:31:38]

I don't know how to test for a leak/failure to ref count correctly like this. As
I said on the review request, there is some implicit coverage for this already.

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:289:
throttlingController()->removeReader(this);
On 2014/02/21 15:25:48, kinuko wrote:
> Is the intention here is to execute RemoveReader's dtor after firing events? 
If
> so don't we need to receive the return value?

No need, the ignored value will be destructed on leaving the scope.

[kinuko, 2014-02-21 15:43:10]

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:289:
throttlingController()->removeReader(this);
On 2014/02/21 15:31:39, sof wrote:
> On 2014/02/21 15:25:48, kinuko wrote:
> > Is the intention here is to execute RemoveReader's dtor after firing events?

> If
> > so don't we need to receive the return value?
> 
> No need, the ignored value will be destructed on leaving the scope.

Hmm, my very rough local testing doesn't say so...

[sof, 2014-02-21 17:01:19]

On 2014/02/21 15:43:10, kinuko wrote:
>
https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
> File Source/core/fileapi/FileReader.cpp (right):
> 
>
https://codereview.chromium.org/174963002/diff/1/Source/core/fileapi/FileRead...
> Source/core/fileapi/FileReader.cpp:289:
> throttlingController()->removeReader(this);
> On 2014/02/21 15:31:39, sof wrote:
> > On 2014/02/21 15:25:48, kinuko wrote:
> > > Is the intention here is to execute RemoveReader's dtor after firing
events?
> 
> > If
> > > so don't we need to receive the return value?
> > 
> > No need, the ignored value will be destructed on leaving the scope.
> 
> Hmm, my very rough local testing doesn't say so...

Oops, my bad - quite right.

[sof, 2014-02-21 17:15:53]

Switched to being explicit about performing this 2nd removal/finishing step
instead.

[sof, 2014-02-23 13:40:25]

On 2014/02/21 17:15:53, sof wrote:
> Switched to being explicit about performing this 2nd removal/finishing step
> instead.

If we instead want to express this with stack allocating a 'removal' object that
performs the two steps in its ctor and dtor, I can switch to that.

But my mild preference is to be explicit about the two steps, like now.

[kinuko, 2014-02-24 02:24:21]

lgtm

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
Source/core/fileapi/FileReader.cpp:78: enum FinishReader {
DoNotRunPendingReaders, DoRunPendingReaders };
nit: can this enum name be FinishReaderType so that it looks more enum-like than
object-like at the callsites?

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
Source/core/fileapi/FileReader.cpp:332: // Unregister the reader
nit: please end comments with period

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
Source/core/fileapi/FileReader.cpp:354: // Unregister the reader
ditto.

[sof, 2014-02-24 07:18:58]

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/174963002/diff/140001/Source/core/fileapi/Fil...
Source/core/fileapi/FileReader.cpp:78: enum FinishReader {
DoNotRunPendingReaders, DoRunPendingReaders };
On 2014/02/24 02:24:21, kinuko wrote:
> nit: can this enum name be FinishReaderType so that it looks more enum-like
than
> object-like at the callsites?

That reads better; thanks, done.

[sof, 2014-02-24 07:19:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-24 07:19:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/174963002/210001

[commit-bot: I haz the power, 2014-02-24 13:32:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/174963002/210001

[commit-bot: I haz the power, 2014-02-24 22:16:19]

Change committed as 167706