Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
55133010:
[PPAPI] Fixed FileSystems from JavaScript not having a context. (Closed)
|
Created:
7 years, 1 month ago by Matt Giuca Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, chrome-apps-syd-reviews_chromium.org, teravest Base URL:
http://git.chromium.org/chromium/src.git@pepper-fs-fileio-test-disable Visibility:
Public. |
Description[PPAPI] Fixed FileSystems from JavaScript not having a context.
Removed the "open existing file system" overload of
PepperFileSystemBrowserHost constructor, replacing it with a method
OpenExisting that asynchronously opens the file system, correctly
obtaining a FileSystemContext.
Rewrote PepperRendererConnection::OnMsgCreateResourceHostsFromHost to
deal with having to call an asynchronous function before sending the
reply to the plugin.
Re-enabled disabled test that was broken due to this bug and added
an additional regression test.
BUG=314007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232605
Patch Set 1 : #
Total comments: 27
Patch Set 2 : Respond to reviews. #Patch Set 3 : Abstract loop body to function to avoid continue. #
Total comments: 2
Patch Set 4 : Revert Patch Set 3. I think it was more readable in-line. #Patch Set 5 : Nit. #
Total comments: 2
Patch Set 6 : Nit: Return early, and un-indent code. #
Messages
Total messages: 21 (0 generated)
Hi guys, This is a last-minute fix for a bug Justin reported today: the PepperFileSystemBrowserHost did not have a context when created from JavaScript. Unfortunately this required a great deal of refactoring to make it work, so this CL (which was supposed to be a quick fix) is now pretty huge. I was hoping to get this all in before the M32 branch point. I realise that if we need to have discussions about this then I won't make it, in which case I will have to revert this entire feature (because it will stop working when Justin submits his CL). Therefore, I would be much obliged if you could have a look at this today. If you just have nits then I can clean it up and send it through in the morning. But if there are major issues with this approach, I am afraid I will have to revert https://codereview.chromium.org/26564009/. (So if there is something you think is acceptable but we can fix up after the branch point, please let me know so I can do it in a follow-up CL.) The one issue I can foresee here is the error handling -- if we don't get a context, I just log a warning and create an invalid resource (which is the same as what I would have created before this CL -- it just errors if you try to use it). I wasn't really sure of a good way to handle errors, since we have to create some resource. I figured this way was acceptable. Apologies for the late notice.
Regarding making m32... branch point is EOD Monday. Were you wanting it public/stable by then? I don't think that's feasible at this point. Unless you have a use of this API that's driving m32, I think it would be better to just relax, take our time, and get it really right in m33. I also think this API could end up as a good guinea pig for us trying "channel-specific APIs" (see https://code.google.com/p/chromium/issues/detail?id=309660). So we'd ideally make it available early in m33 and experiment with it. Assuming everything went well, it would also be made public/stable in m33. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_file_system_browser_host.h (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:55: void OpenExistingHaveContext( nit: s/HaveContext/WithContext ? https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: &PendingHostCreator::AddPendingResourceHost, The idea of this being asynchronous is pretty scary to me. We probably have sequences of operations where we expect that after the creation message has been sent, the pending resource id is immediately usable. In this case, I think it won't be if the FileSystem hasn't been opened yet. I think instead that you need to create the pending resource host immediately, and make *that host* responsible for queueing up any requests that it gets while it's not fully initialized. Once it's fully initialized, *then* it can service those requests. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:207: continue; I think it might be better to structure this differently so you don't need to continue. Maybe put "creator->AddPendingResourceHost" up in the FileRef case explicitly, and take the part that does "host->GetPpapiHost()->CreateResourceHost" below in an else. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:223: // operation will automatically be send to the plugin when |creator| is nit: s/send/sent
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: &PendingHostCreator::AddPendingResourceHost, On 2013/11/01 17:33:52, dmichael wrote: > The idea of this being asynchronous is pretty scary to me. We probably have > sequences of operations where we expect that after the creation message has been > sent, the pending resource id is immediately usable. In this case, I think it > won't be if the FileSystem hasn't been opened yet. > > I think instead that you need to create the pending resource host immediately, > and make *that host* responsible for queueing up any requests that it gets while > it's not fully initialized. Once it's fully initialized, *then* it can service > those requests. Dave's right-- you should definitely call AddPendingResourceHost immediately instead of asynchronously. However, I think you're making this more complicated than it has to be by doing all this work at resource creation time. Can you require that FileSystems received by plugins from the renderer also need a call to Open() to work? Then you can do this asynchronous work there and send an OpenReply message when you're done. The alternative will require a lot of work on the host side to make accessors for GetFileSystemContext asynchronous everywhere. I can help you with this, but it would be messy.
I agree with David and Justin that if there isn't a strong reason, we should take our time to make sure it is right. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: &PendingHostCreator::AddPendingResourceHost, > Dave's right-- you should definitely call AddPendingResourceHost immediately > instead of asynchronously. (Not saying that it is incorrect, just try to understand.) Why do we have to call AddPendingResourceHost immediately instead of asynchronously? I thought the renderer side won't start using the file system before it gets the CreateResourceHostsFromHostReply? And before it is attached, nobody will call the GetFileSystemConext on the resource host, right? > > However, I think you're making this more complicated than it has to be by doing > all this work at resource creation time. > > Can you require that FileSystems received by plugins from the renderer also need > a call to Open() to work? Then you can do this asynchronous work there and send > an OpenReply message when you're done. > > The alternative will require a lot of work on the host side to make accessors > for GetFileSystemContext asynchronous everywhere. I can help you with this, but > it would be messy.
On Fri, Nov 1, 2013 at 3:23 PM, <yzshen@chromium.org> wrote: > I agree with David and Justin that if there isn't a strong reason, we should > take our time to make sure it is right. > > > > https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... > File content/browser/renderer_host/pepper/pepper_renderer_connection.cc > (right): > > https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... > content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: > &PendingHostCreator::AddPendingResourceHost, >> >> Dave's right-- you should definitely call AddPendingResourceHost > > immediately >> >> instead of asynchronously. > > > (Not saying that it is incorrect, just try to understand.) > Why do we have to call AddPendingResourceHost immediately instead of > asynchronously? How do you ensure the plugin doesn't try to attach to the pending resource id before the browser has added it? There's no guarantee about how long that'll take on the browser side with this async code. > I thought the renderer side won't start using the file system before it > gets the CreateResourceHostsFromHostReply? You're right. I guess this could work as is, I hadn't realized the cleverness with refcounts coming from the Bind operation at line 200. > > And before it is attached, nobody will call the GetFileSystemConext on > the resource host, right? Right. > > > >> However, I think you're making this more complicated than it has to be > > by doing >> >> all this work at resource creation time. > > >> Can you require that FileSystems received by plugins from the renderer > > also need >> >> a call to Open() to work? Then you can do this asynchronous work there > > and send >> >> an OpenReply message when you're done. > > >> The alternative will require a lot of work on the host side to make > > accessors >> >> for GetFileSystemContext asynchronous everywhere. I can help you with > > this, but >> >> it would be messy. > > > https://codereview.chromium.org/55133010/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm Yuzhu's conclusion is right; I'd skimmed through this without reading the comments at the top carefully enough. This approach seems a bit complicated, but it should work okay. Generally lgtm.
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: &PendingHostCreator::AddPendingResourceHost, Hey, I'm not sure I understand how this is a problem. My understanding was that OnMsgCreateResourceHostsFromHost is itself called asynchronously -- it is called in response to a message from the renderer. Therefore, the renderer cannot assume that anything done in this function is available immediately; it has to wait until the "Reply" is sent by calling Send. The refactoring I did in this CL was to ensure that Send is not called until after all of the callbacks are called. The (slight) alternative I thought of, which is a bit simpler, was to call PpapiHost::AddPendingResourceHost synchronously, and add it to the vector, and just have the callback do nothing, but delay until the final callback is called before calling Send. But I thought, since the resource host will be unusable anyway, and its ID will be unavailable before the async operation is completed, it is probably safer to only add it as a pending resource host once we get a callback. Either way, I don't see the problem since nobody can see these resource IDs before they are added as pending hosts later. I discussed the option of having the user call Open() briefly with Raymes. I don't really like it, since it is awkward for the user (conceptually: you have to "open" the file system in JavaScript, then pass it to NaCl, then "open" it again). It seems like leaking implementation details to the user if we make the user pay with extra boilerplate steps just because we can't figure out how to do it for them before we send it to them.
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:201: &PendingHostCreator::AddPendingResourceHost, Yeah, I realize now that the sender of CreateFromRenderer doesn't get to see the pending id, so there's no chance of it messing that up. I think it's okay; sorry about that.
Okay, looked again. I think I'd still like the slight restructuring I suggested that would get rid of the "continue". But o/w lgtm
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:159: opened_ = true; Is it intended to set it to true even if fs_context is NULL? https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_file_system_browser_host.h (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:38: void OpenExisting(const GURL& root_url, base::Closure callback); const &, please. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:56: base::Closure callback, const &, please. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:33: : public base::RefCountedThreadSafe<PendingHostCreator> { ThreadSafe is not needed. This code won't be run on multiple thread. (right?) https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:38: int nested_msgs_size, size_t, please. nit, optional: maybe it is better to reorder the parameters: BrowserPpapiHostImpl* host, BrowserMessageFilter* connection, int routing_id, int sequence_id, size_t nested_msgs_size https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:83: connection_->Send(new PpapiHostMsg_CreateResourceHostsFromHostReply( Is it possible that something has failed and we have 0 in the array, what would happen in that case?
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:159: opened_ = true; Good point. I changed it so that opened_ is only true if fs_context is non-NULL. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_file_system_browser_host.h (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:38: void OpenExisting(const GURL& root_url, base::Closure callback); On 2013/11/01 22:27:39, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:55: void OpenExistingHaveContext( On 2013/11/01 17:33:52, dmichael wrote: > nit: s/HaveContext/WithContext > ? Done. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:56: base::Closure callback, On 2013/11/01 22:27:39, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:33: : public base::RefCountedThreadSafe<PendingHostCreator> { It will be passed to another thread, then passed back and run on this thread. I wasn't sure if that required ThreadSafe so I put it in. Or is it only required if it's run simultaneously on two threads? You're right, it's probably not necessary so I'll take out. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:38: int nested_msgs_size, Done. I agree. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:83: connection_->Send(new PpapiHostMsg_CreateResourceHostsFromHostReply( No more possible than it already was. (That is, I'm not sure if PpapiHost::AddPendingResourceHost returns 0, but I haven't introduced any new possibilities of putting 0 in the array.) The asynchronous function always calls the callback. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:207: continue; That's messy. The case currently at the bottom is supposed to be run if any of the if statements fail. Avoiding the continue requires this logic: if (valid instance) { if (message is fileref_createexternal) { if (message is valid) { special case for resource_host default synchronous add } else { // Not sure why an invalid message continues like this, but this // is to preserve the existing behaviour. default case for resource_host default synchronous add } } else if (message is filesystem_createfromrenderer) { special case for resource_host special asynchronous add } else { default case for resource_host default synchronous add } } else { default case for resource_host default synchronous add } There are so many default cases and more will be needed as we add more special cases here. I think the continue is preferable. Instead, I tried moving all of the loop body to a separate function. The continue is now an early return. Do you prefer this? TBH, I think this makes it harder to read because the logic is separated out. So I've uploaded it as a separate Patch Set 3. If you don't like that, we can just go with Patch Set 2. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:223: // operation will automatically be send to the plugin when |creator| is On 2013/11/01 17:33:52, dmichael wrote: > nit: s/send/sent Done.
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:33: : public base::RefCountedThreadSafe<PendingHostCreator> { On 2013/11/01 23:19:47, Matt Giuca wrote: > It will be passed to another thread, then passed back and run on this thread. I > wasn't sure if that required ThreadSafe so I put it in. > > Or is it only required if it's run simultaneously on two threads? You're right, > it's probably not necessary so I'll take out. If the ref needs to be manipulated by multiple threads, we will need RefCountedThreadSafe. In this case, I think it is safe to use RefCounted -- we bind it to a Closure which is eventually run on the same thread. But I am fine if you want to keep RefCountedThreadSafe. https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:83: connection_->Send(new PpapiHostMsg_CreateResourceHostsFromHostReply( On 2013/11/01 23:19:47, Matt Giuca wrote: > No more possible than it already was. (That is, I'm not sure if > PpapiHost::AddPendingResourceHost returns 0, but I haven't introduced any new > possibilities of putting 0 in the array.) The asynchronous function always calls > the callback. I think it is possible: The FileSystem resource host may be destroyed before OpenExisting() completes, in that case, OpenExistingWithContext() won't be called, and PendingHostCreator::AddPendingResourceHost() won't be called either. In the destructor, we will send 0 to the renderer. https://codereview.chromium.org/55133010/diff/190001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/190001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:215: BrowserPpapiHostImpl* host = GetHostForChildProcess(child_process_id); You could move line 220 - 221 here and return directly if host == NULL, so that you won't pass a NULL pointer into |creator|.
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:33: : public base::RefCountedThreadSafe<PendingHostCreator> { I agree. RefCounted seems fine. https://codereview.chromium.org/55133010/diff/190001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/190001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:215: BrowserPpapiHostImpl* host = GetHostForChildProcess(child_process_id); On 2013/11/02 00:25:07, yzshen1 wrote: > You could move line 220 - 221 here and return directly if host == NULL, so that > you won't pass a NULL pointer into |creator|. Done.
LGTM, with one more comment. https://codereview.chromium.org/55133010/diff/320001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/320001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:162: DLOG(ERROR) << "Invalid plugin process ID."; please directly return, then we don't have to have all the other things indented in the 'else'-block.
https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/30001/content/browser/renderer_... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:83: connection_->Send(new PpapiHostMsg_CreateResourceHostsFromHostReply( Had a chat with Yuzhu. I agree, there is a very rare chance that this will happen if the plugin dies while the host is being created. In my quick testing of this, passing 0 as the pending host ID may crash the plugin, but if the plugin is dead this shouldn't matter. We decided this was not worth holding up the fix. Filed as http://crbug.com/314289. https://codereview.chromium.org/55133010/diff/320001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/55133010/diff/320001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:162: DLOG(ERROR) << "Invalid plugin process ID."; On 2013/11/02 00:54:01, yzshen1 wrote: > please directly return, then we don't have to have all the other things indented > in the 'else'-block. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/55133010/240002
Retried try job too often on win_rel for step(s) content_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/55133010/240002
Retried try job too often on win_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/55133010/240002
Message was sent while issue was closed.
Change committed as 232605 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
