[FileAPI] Check Platform availability before using it in LocalFileSystem

[FileAPI] Check Platform availability before using it in LocalFileSystem

BUG=399803
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179911

[tzik, 2014-08-11 05:03:19]

PTL

[tzik, 2014-08-11 05:25:36]

Updated, as we chatted offline.

[nhiroki, 2014-08-11 05:31:52]

LGTM with nits

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:117: Platform* platform =
blink::Platform::current();
nit: "blink::" is not necessary?

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:120: return
blink::Platform::current()->fileSystem();
ditto.

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:77: void
fileSystemUnavailable(PassRefPtrWillBeRawPtr<ExecutionContext>,
PassRefPtr<CallbackWrapper>);
nit, optional: "fileSystemNotAvailable" would be more consistent with
"fileSYstemNotAllowed".

[tzik, 2014-08-11 06:18:46]

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:117: Platform* platform =
blink::Platform::current();
On 2014/08/11 05:31:52, nhiroki wrote:
> nit: "blink::" is not necessary?

Done.

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:120: return
blink::Platform::current()->fileSystem();
On 2014/08/11 05:31:52, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/458093002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.h:77: void
fileSystemUnavailable(PassRefPtrWillBeRawPtr<ExecutionContext>,
PassRefPtr<CallbackWrapper>);
On 2014/08/11 05:31:52, nhiroki wrote:
> nit, optional: "fileSystemNotAvailable" would be more consistent with
> "fileSYstemNotAllowed".

Done.

[tzik, 2014-08-11 06:26:40]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-08-11 06:27:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tzik@chromium.org/458093002/40001

[commit-bot: I haz the power, 2014-08-11 07:24:23]

Change committed as 179911

[kinuko, 2014-08-11 16:03:49]

DBC

https://codereview.chromium.org/458093002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/458093002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:120: return
Platform::current()->fileSystem();
Is the crash happening because Platform::shutdown() is called on the main thread
before we access Platform::current() on the worker thread (do we still have
not-joining worker thread problems)?  Is it guaranteed it's not nullified
between line 117 and where we use fileSystem()?  (Sorry if I'm asking something
outdated)

[tzik, 2014-08-12 06:12:11]

https://codereview.chromium.org/458093002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/458093002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/LocalFileSystem.cpp:120: return
Platform::current()->fileSystem();
On 2014/08/11 16:03:49, kinuko (slow to respond) wrote:
> Is the crash happening because Platform::shutdown() is called on the main
thread
> before we access Platform::current() on the worker thread (do we still have
> not-joining worker thread problems)?  Is it guaranteed it's not nullified
> between line 117 and where we use fileSystem()?  (Sorry if I'm asking
something
> outdated)

Right, we still can't assume all workers join to the main thread in the shutdown
phase. And, yes, Platform::shutdown() and this Platform::current() check look
racy :<