Gracefully handle FileReader() read operations when in a detached state.

Gracefully handle FileReader() read operations when in a detached state.

Follow up r178274 and have FileReader handle the case when it becomes
detached from its document and its frame. Read operations can no longer
go ahead when that is the case, as the throttling controller isn't
available.

R=haraken@chromium.org,horo@chromium.org,nhiroki@chromium.org,kinuko@chromium.org
BUG=394828
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178426

[sof, 2014-07-17 11:23:33]

Please take a look.

I realized that I had failed to handle the case of frame supplements becoming
detached; sorry.

Uncommon state to be in, so I ended up simply throwing. An alternative is to
perform the read operation without consulting a throttling controller (as there
isn't one); not ideal that either.

[haraken, 2014-07-17 12:44:16]

LGTM, but nhiroki@ or tzik@ wants to confirm (especially if NotSupportedError is
a right error to throw).

[kinuko, 2014-07-17 15:22:24]

(No need to wait for my further comments)

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

https://codereview.chromium.org/399863002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:292:
exceptionState.throwDOMException(NotSupportedError, "Reading from a
Document-detached FileReader is not supported.");
I don't have strong opinion, but AbortError might sound more common in these
cases?

https://codereview.chromium.org/399863002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:403: ThrottlingController::FinishReaderType
finalStep = throttlingController()->removeReader(this);
I assume these loader client methods are called while execution context is alive
(is it guaranteed?)

[sof, 2014-07-17 15:42:45]

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

https://codereview.chromium.org/399863002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:403: ThrottlingController::FinishReaderType
finalStep = throttlingController()->removeReader(this);
On 2014/07/17 15:22:24, kinuko (ooo) wrote:
> I assume these loader client methods are called while execution context is
alive
> (is it guaranteed?)

Good question; my assumption was that a valid ExecutionContext implied that the
LocalFrame would stay around. But that sounds far fetched, will go over it again
tomorrow.

[sof, 2014-07-17 22:12:01]

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

https://codereview.chromium.org/399863002/diff/1/Source/core/fileapi/FileRead...
Source/core/fileapi/FileReader.cpp:403: ThrottlingController::FinishReaderType
finalStep = throttlingController()->removeReader(this);
On 2014/07/17 15:42:44, sof wrote:
> On 2014/07/17 15:22:24, kinuko (ooo) wrote:
> > I assume these loader client methods are called while execution context is
> alive
> > (is it guaranteed?)
> 
> Good question; my assumption was that a valid ExecutionContext implied that
the
> LocalFrame would stay around. But that sounds far fetched, will go over it
again
> tomorrow.

kinuko-san, thanks for raising the lifetime question (and I really shouldn't
bother you when you're OOO; will try not to :) ).

I've rephrased how the FileReader accesses its throttling controller, leaving it
to its implementation to safely handle the details of whether or not the
underlying supplement instance is available.

[haraken, 2014-07-18 01:01:54]

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
Source/core/fileapi/FileReader.cpp:321:
exceptionState.throwDOMException(InvalidStateError, "Reading from a
Document-detached FileReader is not supported.");

Looks like kinuko-san prefers AbortError?

[nhiroki, 2014-07-18 01:54:43]

lgtm

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
Source/core/fileapi/FileReader.cpp:321:
exceptionState.throwDOMException(InvalidStateError, "Reading from a
Document-detached FileReader is not supported.");
On 2014/07/18 01:01:53, haraken wrote:
> 
> Looks like kinuko-san prefers AbortError?

I'd prefer AbortError, too.

[horo, 2014-07-18 03:12:15]

lgtm

[sof, 2014-07-18 05:26:37]

haraken@: the final version looks acceptable to you too?

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/399863002/diff/20001/Source/core/fileapi/File...
Source/core/fileapi/FileReader.cpp:321:
exceptionState.throwDOMException(InvalidStateError, "Reading from a
Document-detached FileReader is not supported.");
On 2014/07/18 01:54:42, nhiroki wrote:
> On 2014/07/18 01:01:53, haraken wrote:
> > 
> > Looks like kinuko-san prefers AbortError?
> 
> I'd prefer AbortError, too.

ok, let's throw that instead. Switched over.

[haraken, 2014-07-18 05:28:40]

> haraken@: the final version looks acceptable to you too?

Yes, LGTM.

[sof, 2014-07-18 05:32:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 05:33:33]

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

[commit-bot: I haz the power, 2014-07-18 06:36:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17636)

[commit-bot: I haz the power, 2014-07-18 06:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 06:50:31]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17636)

[sof, 2014-07-18 07:01:14]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 07:02:02]

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

[commit-bot: I haz the power, 2014-07-18 08:02:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17650)

[commit-bot: I haz the power, 2014-07-18 08:40:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 08:40:25]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17661)

[sof, 2014-07-18 08:45:35]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-18 08:46:31]

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

[commit-bot: I haz the power, 2014-07-18 09:21:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17672)

[commit-bot: I haz the power, 2014-07-18 09:51:24]

Change committed as 178426