Implement Worker-MainThread bridge for FileSystem API in Chrome

Implement Worker-MainThread bridge for FileSystem API in Chrome

This basically adds the same worker/mainThread bridge functionality
for FileSystem API in Chrome side.

Keeping bridge code around is suboptimal, but this allows us to remove
bridge code in Blink side which in turn allows us to make further
worker-related cleanups in Blink.

This change doesn't contain FileWriter related changes yet.
(For reference, combined patch is uploaded here:
https://codereview.chromium.org/19287010/)

BUG=257349
TEST=no behavioral changes yet
R=atwilson@chromium.org, jochen@chromium.org, michaeln@chromium.org, tzik@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214160

[kinuko, 2013-07-16 14:35:43]

Michael, as I wrote in other issue I ended up implementing bridge code in Chrome
side (to get rid of the similar but more complex bridge code from Blink).  Can
you take a look, especially around CreateSnapshotFile handling?  (I'll ask tzik
or ericu for the second review later)

Thanks,

[kinuko, 2013-07-22 02:22:40]

Michael seems busy or not around.
tzik: would you be able to do the fileapi-specific review?

This patch is basically porting what WorkerFileSystemCallbacksBridge does in
Blink side to Chrome side).  The brief plan is outlined in
https://docs.google.com/a/chromium.org/document/d/139oqf212hzi3_r8K_GfTsiiiZa...

[tzik, 2013-07-22 04:18:23]

lgtm

[kinuko, 2013-07-22 04:51:01]

Avi, can you take a brief look at /content/{renderer,worker} changes?

[kinuko, 2013-07-23 01:00:46]

(-avi)

jochen, atwilson: can you review /content/{renderer,worker} changes?

[jochen (gone - plz use gerrit), 2013-07-23 11:02:48]

content/renderer lgtm

[Andrew T Wilson (Slow), 2013-07-23 13:03:20]

content/worker lgtm

[michaeln, 2013-07-24 00:54:08]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
File content/child/fileapi/file_system_dispatcher.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:20: base::MessageLoopProxy*
message_loop) {
You might be able to use the ThreadSafeSender here instead of manually
loop->PostTask'ing in this function.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:163:
dispatcher->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
O/T for this CL, but similarly we're shutting down if we're destructing (right),
so I wonder about not attempting to propagate DidFail() back to script at this
point.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:68: virtual void
OnWorkerRunLoopStopped() OVERRIDE {
At this point the worker run loop is wound down and the script execution
context is all but gone. Is there any reason to complete/finish the call in this
case, or would it work to just stop processing here with an early return?

If the call to didFail() results in webcore logic trying to create new v8
objects (exceptions) or invoke methods of v8 objects, i think that's an
opportunity for odd/unexpected things happening.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:150:
callbacks->didFail(WebKit::WebFileErrorAbort);
It looks like didFail() can be called on the main thread even if the callbacks
have worker thread affinity.

I think in cases where there is no ChildThread or fs_dispatcher instance, that's
indicative of being in the midst of shut down. Is that right? Would it work to
just early return here and stop executing more code in this case.

[michaeln, 2013-07-24 01:38:58]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
File content/child/fileapi/file_system_dispatcher.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:113:
snapshot_callback_.Run(file_info, platform_path, did_receive_snapshot);
What method/function is snapshot_callback_ this wired to? I see,
CallbacksWrapper::DidCreateSnapshotFile.

Alternatively, instead of passing a did_receive_snapshot Closure to be invoked
by that method, maybe alter that method to directly call...
  sender->Send(new FileSystemHostMsg_DidReceiveSnapshotFile(request_id));
...i guess you'd also have to pass in the request_id to that method in place of
this closure.

The closure indirection is harder to follow fwiw.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:68: virtual void
OnWorkerRunLoopStopped() OVERRIDE {
On 2013/07/24 00:54:08, michaeln wrote:
> At this point the worker run loop is wound down and the script execution
> context is all but gone. Is there any reason to complete/finish the call in
this
> case, or would it work to just stop processing here with an early return?

Or i guess what i meant to say was, maybe just {
  delete this;
}

[michaeln, 2013-07-24 06:19:12]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:70: delete this;
Oh, I have another question about this method?

Pointers to CallbackWrapper instances are given to the main
FileSystemDispatcher. When that main dispatcher receives a response from the
browser process, it invokes the CallbackWrapper's appropriate
WebFileSystemCallbacks override. But if OnWorkerRunLoopStopped happens prior to
having received that response (and delete this has happened), i think we'll hit
a UAF bug when the FileSystemDispatcher tries to use it.

[kinuko, 2013-07-24 09:29:09]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
File content/child/fileapi/file_system_dispatcher.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:20: base::MessageLoopProxy*
message_loop) {
On 2013/07/24 00:54:08, michaeln wrote:
> You might be able to use the ThreadSafeSender here instead of manually
> loop->PostTask'ing in this function.

Done.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:113:
snapshot_callback_.Run(file_info, platform_path, did_receive_snapshot);
On 2013/07/24 01:38:58, michaeln wrote:
> What method/function is snapshot_callback_ this wired to? I see,
> CallbacksWrapper::DidCreateSnapshotFile.
> 
> Alternatively, instead of passing a did_receive_snapshot Closure to be invoked
> by that method, maybe alter that method to directly call...
>   sender->Send(new FileSystemHostMsg_DidReceiveSnapshotFile(request_id));
> ...i guess you'd also have to pass in the request_id to that method in place
of
> this closure.
> 
> The closure indirection is harder to follow fwiw.

I thought returning a closure is clearer about what is expected, but it might be
also true that adding more indirection makes things harder to follow... ok,
done.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.cc:163:
dispatcher->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
On 2013/07/24 00:54:08, michaeln wrote:
> O/T for this CL, but similarly we're shutting down if we're destructing
(right),
> so I wonder about not attempting to propagate DidFail() back to script at this
> point.

Current API contract expects it to be called in the end, so let me keep this as
is for now.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:68: virtual void
OnWorkerRunLoopStopped() OVERRIDE {
On 2013/07/24 01:38:58, michaeln wrote:
> On 2013/07/24 00:54:08, michaeln wrote:
> > At this point the worker run loop is wound down and the script execution
> > context is all but gone. Is there any reason to complete/finish the call in
> this
> > case, or would it work to just stop processing here with an early return?
> 
> Or i guess what i meant to say was, maybe just {
>   delete this;
> }

Current API is designed as 1. the callback is self-destruct and 2. one of the
methods must be called by the caller.
Deleting callback_ is API violation, but leaking callback_ could leak more
objects in the blink side. In the end the callback does nothing after the script
context's shutdown, so calling this should be safe. (Or at least that's what
we've been doing in Blink)

I'm wondering if I should change this API contract (to let the caller explicitly
delete the callback), but let me keep this code as is for now.

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:70: delete this;
On 2013/07/24 06:19:13, michaeln wrote:
> Oh, I have another question about this method?
> 
> Pointers to CallbackWrapper instances are given to the main
> FileSystemDispatcher. When that main dispatcher receives a response from the
> browser process, it invokes the CallbackWrapper's appropriate
> WebFileSystemCallbacks override. But if OnWorkerRunLoopStopped happens prior
to
> having received that response (and delete this has happened), i think we'll
hit
> a UAF bug when the FileSystemDispatcher tries to use it.

Ah, in the main thread side-- yes, good catch.  Fixed (I think).

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:150:
callbacks->didFail(WebKit::WebFileErrorAbort);
On 2013/07/24 00:54:08, michaeln wrote:
> It looks like didFail() can be called on the main thread even if the callbacks
> have worker thread affinity.
> 
> I think in cases where there is no ChildThread or fs_dispatcher instance,
that's
> indicative of being in the midst of shut down. Is that right? Would it work to
> just early return here and stop executing more code in this case.

Done.

[kinuko, 2013-07-24 11:03:29]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:68: virtual void
OnWorkerRunLoopStopped() OVERRIDE {
On 2013/07/24 09:29:09, kinuko wrote:
> On 2013/07/24 01:38:58, michaeln wrote:
> > On 2013/07/24 00:54:08, michaeln wrote:
> > > At this point the worker run loop is wound down and the script execution
> > > context is all but gone. Is there any reason to complete/finish the call
in
> > this
> > > case, or would it work to just stop processing here with an early return?
> > 
> > Or i guess what i meant to say was, maybe just {
> >   delete this;
> > }
> 
> Current API is designed as 1. the callback is self-destruct and 2. one of the
> methods must be called by the caller.
> Deleting callback_ is API violation, but leaking callback_ could leak more
> objects in the blink side. In the end the callback does nothing after the
script
> context's shutdown, so calling this should be safe. (Or at least that's what
> we've been doing in Blink)
> 
> I'm wondering if I should change this API contract (to let the caller
explicitly
> delete the callback), but let me keep this code as is for now.

Well... on the second look the current API seems to expect callbacks to be
called or deleted (though it's not documented well).  Ok, let's just delete the
callbacks here.

[kinuko, 2013-07-24 11:15:41]

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/32001/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.cc:68: virtual void
OnWorkerRunLoopStopped() OVERRIDE {
On 2013/07/24 11:03:29, kinuko wrote:
> On 2013/07/24 09:29:09, kinuko wrote:
> > On 2013/07/24 01:38:58, michaeln wrote:
> > > On 2013/07/24 00:54:08, michaeln wrote:
> > > > At this point the worker run loop is wound down and the script execution
> > > > context is all but gone. Is there any reason to complete/finish the call
> in
> > > this
> > > > case, or would it work to just stop processing here with an early
return?
> > > 
> > > Or i guess what i meant to say was, maybe just {
> > >   delete this;
> > > }
> > 
> > Current API is designed as 1. the callback is self-destruct and 2. one of
the
> > methods must be called by the caller.
> > Deleting callback_ is API violation, but leaking callback_ could leak more
> > objects in the blink side. In the end the callback does nothing after the
> script
> > context's shutdown, so calling this should be safe. (Or at least that's what
> > we've been doing in Blink)
> > 
> > I'm wondering if I should change this API contract (to let the caller
> explicitly
> > delete the callback), but let me keep this code as is for now.
> 
> Well... on the second look the current API seems to expect callbacks to be
> called or deleted (though it's not documented well).  Ok, let's just delete
the
> callbacks here.

Oops, callbacks can't be deleted from outside since dtor is protected. Well...
let me keep these didFail(ABORT) calls as is for now in this CL. I'll revisit
this issue later

[michaeln, 2013-07-24 22:21:12]

See my 2nd inline comment in particular.

> Oops, callbacks can't be deleted from outside since dtor is protected. Well...
> let me keep these didFail(ABORT) calls as is for now in this CL. I'll revisit
> this issue later
yup, sg

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:122: new
FileSystemHostMsg_DidReceiveSnapshotFile(request_id));
Drat, I think i gave you a bum steer, very sorry :(

The code as written in this snapshot is what should be good, but since blob msgs
(that happen under the call to didCreateSnapshotFile) still employ main thread
bridging while ThreadSafeSender does not, it's racey.

Your earlier code to have the ACK message go thru the 'main' thread had it right
for now. You can add a TODO(michaeln) on the code routing the msg thru the main
thread to change it after blob msgs are switched over.

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:139: delete this;
I'm really not sure about 'delete this' correctness in this case? Clearly, if
the worker runloop is well past having stopped, this delete is fine and good.

But I'm wondering about intermediary states in the act of worker shutdown where
the runloop is not yet stopped but it's internal message queue has stopped
accepting new message. I think that is an expected case based on reading thru
some code and we'll see 'false' here.

I don't think a false return from PostTask strictly means the
OnWorkerRunLoopStopped() has been called, but since the object ptr is not
removed as a stop observer in this delete, it will get called. If that's right
it'll be UAF at that time.

This is a bit tricky.

[kinuko, 2013-07-25 04:32:41]

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:122: new
FileSystemHostMsg_DidReceiveSnapshotFile(request_id));
On 2013/07/24 22:21:12, michaeln wrote:
> Drat, I think i gave you a bum steer, very sorry :(
> 
> The code as written in this snapshot is what should be good, but since blob
msgs
> (that happen under the call to didCreateSnapshotFile) still employ main thread
> bridging while ThreadSafeSender does not, it's racey.
> 
> Your earlier code to have the ACK message go thru the 'main' thread had it
right
> for now. You can add a TODO(michaeln) on the code routing the msg thru the
main
> thread to change it after blob msgs are switched over.

Yup... I vaguely remember that's how I decided to use main thread model in the
initial patch, though I had forgotten about that.

Done.

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:139: delete this;
On 2013/07/24 22:21:12, michaeln wrote:
> I'm really not sure about 'delete this' correctness in this case? Clearly, if
> the worker runloop is well past having stopped, this delete is fine and good.
> 
> But I'm wondering about intermediary states in the act of worker shutdown
where
> the runloop is not yet stopped but it's internal message queue has stopped
> accepting new message. I think that is an expected case based on reading thru
> some code and we'll see 'false' here.
> 
> I don't think a false return from PostTask strictly means the
> OnWorkerRunLoopStopped() has been called, but since the object ptr is not
> removed as a stop observer in this delete, it will get called. If that's right
> it'll be UAF at that time.
> 
> This is a bit tricky.

Right. I overlooked PostTask returns false when WorkerRunLoop.postTask
(MessageQueue.append) returns false, which could happen before RunLoopStopped. 
Ok... I could add a locked flag, but I'd like to leak callbacks in the case.
(There're some other places we leak callbacks in the shutdown sequence)

[michaeln, 2013-07-25 21:39:08]

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:139: delete this;
> Right. I overlooked PostTask returns false when WorkerRunLoop.postTask
> (MessageQueue.append) returns false, which could happen before RunLoopStopped.

> Ok... I could add a locked flag, but I'd like to leak callbacks in the case.
> (There're some other places we leak callbacks in the shutdown sequence)

For a process shutdown leaks, who cares, but these are potential
per-worker-thread leaks. I don't think we should leave leaks at this level in
permanently. If we leave the leak in for now, tools will probably yell at us
some, and we should probably have a bug and a TODO referring to the bug.

IDB avoids this sort of problem by having a somewhat different split. In
IDB'land there is a single stateless MessageFilter class (lives on the IPC
thread) and there are per-thread stateful Dispatcher instances (live on context
threads). The Dispatcher instances and all per-call Callback like data
structures are 100% owned/accessed only the single thread, ptrs values to the
thread specific data are not ever seen on other threads. Key to allowing that to
work is that the the target thread_id/worker_id is baked into the IPC msgs and
responses. The Filter knows what to do with the response purely based on data
contained in the msg. If a worker runloops stops the Dispatcher and all pending
response statefulness can be deleted without concern for what the Filter is
doing atm or will do at a later time.

I don't know if you're working towards having the FileAPI function along those
lines or not? But having very strictly apartment threaded stuff seems to help.

[kinuko, 2013-07-26 12:29:55]

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/19387002/diff/100002/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:139: delete this;
On 2013/07/25 21:39:08, michaeln wrote:
> > Right. I overlooked PostTask returns false when WorkerRunLoop.postTask
> > (MessageQueue.append) returns false, which could happen before
RunLoopStopped.
> 
> > Ok... I could add a locked flag, but I'd like to leak callbacks in the case.
> > (There're some other places we leak callbacks in the shutdown sequence)
> 
> For a process shutdown leaks, who cares, but these are potential
> per-worker-thread leaks. I don't think we should leave leaks at this level in
> permanently. If we leave the leak in for now, tools will probably yell at us
> some, and we should probably have a bug and a TODO referring to the bug.
> 
> IDB avoids this sort of problem by having a somewhat different split. In
> IDB'land there is a single stateless MessageFilter class (lives on the IPC
> thread) and there are per-thread stateful Dispatcher instances (live on
context
> threads). The Dispatcher instances and all per-call Callback like data
> structures are 100% owned/accessed only the single thread, ptrs values to the
> thread specific data are not ever seen on other threads. Key to allowing that
to
> work is that the the target thread_id/worker_id is baked into the IPC msgs and
> responses. The Filter knows what to do with the response purely based on data
> contained in the msg. If a worker runloops stops the Dispatcher and all
pending
> response statefulness can be deleted without concern for what the Filter is
> doing atm or will do at a later time.
> 
> I don't know if you're working towards having the FileAPI function along those
> lines or not? But having very strictly apartment threaded stuff seems to help.

My initial implementation was exactlly following IDB's model, made Dispatcher
thread-local instance and had another MessageFilter to relay the response to the
correct caller thread ID. But then it turned out that we still need worker to
main-thread bridging due to Blob and API compatibility issues, so I went back to
intermediary bridge model like the one in Blink, hoping eventually we could
migrate to simpler model.  In the sense answer is yes.

I've revised this once again, added thread-local callbacks map (currently this
cannot be held by WebFileSystemImpl nor FileSystemDispatcher) and made the
bridge code trigger callbacks via this map.  Now we have more indirection, but
thread separation must be clearer. Can you take another look?

[michaeln, 2013-07-26 20:19:07]

LGTM!

[kinuko, 2013-07-29 02:55:30]

Thanks for reviewing!

[commit-bot: I haz the power, 2013-07-29 03:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/19387002/232001

[commit-bot: I haz the power, 2013-07-29 04:23:42]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[kinuko, 2013-07-29 05:49:12]

Committed patchset #7 manually as r214160 (presubmit successful).