Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26564009:
[PPAPI] It is now possible to pass filesystems from JavaScript to NaCl modules. (Closed)
|
Created:
7 years, 2 months ago by Matt Giuca Modified:
7 years, 1 month ago Reviewers:
dmichael (off chromium), tsepez (do not use), palmer, kinuko, teravest, yzshen1, Tom Sepez, raymes CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, chrome-apps-syd-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Description[PPAPI] It is now possible to pass filesystems from JavaScript to NaCl modules.
If a DOMFileSystem is passed as a message to the NaCl module, it will be
converted into a resource var which is available to the plugin via the dev
interface PPB_VarResource_Dev.
BUG=177017
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232080
Patch Set 1 #
Total comments: 56
Patch Set 2 : Respond to some of the comments. #Patch Set 3 : Added comments. #Patch Set 4 : Remove unused ResourceCreationProxy proxy. #Patch Set 5 : Wrote browser test. #Patch Set 6 : Rebase. #Patch Set 7 : ResourceRawVarData: Allow pp_resource_ to be 0; misc cleanup. #Patch Set 8 : ResourceRawVarData: Remove ResourceDispatcher and inline everything; much simpler. #
Total comments: 17
Patch Set 9 : Nits. #Patch Set 10 : Test: Write to a temporary file in JS and read it in the plugin. #Patch Set 11 : GetConnectionForInstance DCHECKs if in-process. #
Total comments: 7
Patch Set 12 : David's testing suggestions. #Patch Set 13 : ResourceConverter fails gracefully when given an EXTERNAL file system. #
Total comments: 13
Patch Set 14 : Raymes nits. #Patch Set 15 : Rebase. #Patch Set 16 : Move code from ResourceRawVarData to PluginVarTracker (work around deps issues). #Patch Set 17 : Cleanup after refactor. #
Total comments: 12
Patch Set 18 : Use a vector instead of variable-length array. (Fix Windows compile). #
Total comments: 4
Patch Set 19 : Raymes nits. #
Total comments: 9
Patch Set 20 : Address comments. #Messages
Total messages: 45 (0 generated)
Hi, this is the final piece of the puzzle to get file passing from JS to NaCl to work. (Still to come: C++ wrapper and NaCl to JS passing.) Please have a look. No tests yet, I haven't figured out what bits need to be tested and how to do it, but it will probably need browser tests. Raymes, you mentioned that you had some tests ready for this, perhaps we can integrate them or you can just commit them after. I need a security review for ppapi_messages. I'll get LGTMs from you guys first then add someone from security.
looking good, thanks! https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:114: } else if (nested_msg.type() == PpapiHostMsg_FileSystem_CreateFromRenderer::ID) { nit: long line https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:34: // Returns true on success, false on failure. Seems like this comment doesn't match the behavior. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:36: WebKit::WebFileSystem::Type type) { Does this fit on the previous line? https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:46: } Do we need a default case for safety here? https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:66: content::PepperFileSystemHost* file_system_host = nit: maybe initialize a scoped_ptr here to make ownership clear https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:119: return false; nit: usually use { with multi-line if's https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:123: *result = result_var->GetPPVar(); optional: is it clearer to combine the previous two statements? https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/ppapi_messages.h#... ppapi/proxy/ppapi_messages.h:1303: PP_FileSystemType /* file_system_type */) Could you fully document both of these? In particular state what action they will trigger when they are received, which channel they are passed over and what the parameters are for. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:677: class ResourceDispatcher { This class seems like a little overkill for what's needed here and is a little confusing for me. I think we only need 2 functions: 1) Connection GetConnection(PP_Instance); which returns a connection. 2) void ResourceRawVarData::HandleCreationMessage(const IPC::Message& msg); which has a switch(msg.type()) {} and unpacks the creation message in a similar way to content_renderer_pepper_host_factory.cc. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:681: ResourceDispatcher(PP_Instance instance, int pending_renderer_id, nit: one line per argument https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:790: } Have you considered whether all these DCHECKs should be hard failures? Should we expect to trust the values of all those variables?
On 2013/10/10 05:15:38, Matt Giuca wrote: > Hi, this is the final piece of the puzzle to get file passing from JS to NaCl to > work. (Still to come: C++ wrapper and NaCl to JS passing.) > > Please have a look. No tests yet, I haven't figured out what bits need to be > tested and how to do it, but it will probably need browser tests. Raymes, you > mentioned that you had some tests ready for this, perhaps we can integrate them > or you can just commit them after. I haven't written any tests but I had a skeleton for testing the ResourceConverter which I've lost since we chatted :( We can discuss this more. > > I need a security review for ppapi_messages. I'll get LGTMs from you guys first > then add someone from security.
https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { if this if-block doesn't run, |result| is not touched and we return true, is that intended? https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:711: DCHECK(PpapiGlobals::Get()->IsPluginGlobals()); Please see my comment on line 779. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:734: ResourceCreationProxy proxy(dispatcher_); Can we just EnterResourceCreationNoLock without explicitly handling |dispatcher_| and ResourceCreationProxy and GetConnection? https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { If a plugin is run "in-process", the plugin side of logic is also run in the renderer process, but in this case I think IsPluginGlobals() will return false. Dispatcher::IsPlugin() may be the right answer.
https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:124: file_system_type)); Hmm. This could actually go in to content_browser_pepper_host_factory.cc. FileRef_CreateExternal is special not only because it's created from the renderer, but also because it could be a security vulnerability if it could be faked from the plugin. But I can see the logic in putting it here. If we leave it here, maybe you can add a comment similar to the one at 102, or something. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:59: PP_FileSystemType file_system_type; would be better to initialize this immediately. You can probably just move the declaration down to where you call WebFileSystemTypeToPPAPI https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:75: file_system_type); Maybe it would make sense to use scoped_ptr<IPC::Message>* (i.e., a scoped_ptr out-param.) Every time you copy a Message, it has to allocate the buffer and copy the bytes. That can be somewhat expensive, especially for big messages. Using a scoped_ptr out-param, you can allocate it once and (almost) never have to copy. You could extend that all the way down to HostResourceVar having a scoped_ptr<IPC::Message>. This might be a bit much to throw in to this CL, so maybe just consider it as a future optimization. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:77: *browser_host_create_message = (same suggestion for this message) https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { On 2013/10/11 17:45:45, yzshen1 wrote: > if this if-block doesn't run, |result| is not touched and we return true, is > that intended? +1. I think we should leave |result| untouched and return false.
Responding to some of the review comments. The more complex ones will take a bit more thinking. https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:114: } else if (nested_msg.type() == PpapiHostMsg_FileSystem_CreateFromRenderer::ID) { On 2013/10/11 04:02:24, raymes wrote: > nit: long line Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:34: // Returns true on success, false on failure. On 2013/10/11 04:02:24, raymes wrote: > Seems like this comment doesn't match the behavior. Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:36: WebKit::WebFileSystem::Type type) { On 2013/10/11 04:02:24, raymes wrote: > Does this fit on the previous line? Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:46: } On 2013/10/11 04:02:24, raymes wrote: > Do we need a default case for safety here? Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:59: PP_FileSystemType file_system_type; On 2013/10/11 19:21:36, dmichael wrote: > would be better to initialize this immediately. You can probably just move the > declaration down to where you call WebFileSystemTypeToPPAPI Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:66: content::PepperFileSystemHost* file_system_host = That doesn't work; scoped_ptr can't be copied so it has to be initialized in the argument list. Actually, I'll just inline the call to "new PepperFileSystemHost". It is a bit messier but it avoids having a raw pointer sitting around. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { Yes, this was intentional (but perhaps not the best design). It's trickier than that. See the comment here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... There are basically three outcomes: - Success. - Not a resource. - Unexpected error. The "not a resource" case cannot be considered an error. We have no way to tell whether a V8 value is going to convert to a resource until we call this method. If it's a resource, v8_var_converter will proceed to treat the V8 value as an ordinary dictionary. If it's an error, v8_var_converter will error out. This logic is already committed, and it's a pain to change it (if you want to change it, it's outside the scope of this CL). I agree, it's not ideal to have the function return true but leave some inputs unchanged. A quick fix would be to simply set |result| to PP_MakeNull() in the "not a resource" case. Is that acceptable? https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:119: return false; On 2013/10/11 04:02:24, raymes wrote: > nit: usually use { with multi-line if's Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:123: *result = result_var->GetPPVar(); I don't think so, because then you'll never see that it was a scoped_refptr. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:677: class ResourceDispatcher { The reason I needed a class (instead of just functions) is because of the way IPC_BEGIN_MESSAGE_MAP works. It needs to take a class, and calls a method on the class. It didn't seem to work with a closure; since OnMsgFileSystem_CreateFromPendingHost requires some outside state, the only way to pass it is via the members of a class. (Or so it would seem when I wrote this; it was some time ago.) https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:681: ResourceDispatcher(PP_Instance instance, int pending_renderer_id, On 2013/10/11 04:02:24, raymes wrote: > nit: one line per argument Done.
https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:66: content::PepperFileSystemHost* file_system_host = nit: You could call Pass() on the scoped_ptr if you want to create it outside of the argument list. On 2013/10/15 00:25:16, Matt Giuca wrote: > That doesn't work; scoped_ptr can't be copied so it has to be initialized in the > argument list. > > Actually, I'll just inline the call to "new PepperFileSystemHost". It is a bit > messier but it avoids having a raw pointer sitting around. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { More comment sounds like a good idea. :) On 2013/10/15 00:25:16, Matt Giuca wrote: > Yes, this was intentional (but perhaps not the best design). It's trickier than > that. See the comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... > > There are basically three outcomes: > - Success. > - Not a resource. > - Unexpected error. > > The "not a resource" case cannot be considered an error. We have no way to tell > whether a V8 value is going to convert to a resource until we call this method. > If it's a resource, v8_var_converter will proceed to treat the V8 value as an > ordinary dictionary. If it's an error, v8_var_converter will error out. This > logic is already committed, and it's a pain to change it (if you want to change > it, it's outside the scope of this CL). > > I agree, it's not ideal to have the function return true but leave some inputs > unchanged. A quick fix would be to simply set |result| to PP_MakeNull() in the > "not a resource" case. Is that acceptable?
https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:123: *result = result_var->GetPPVar(); It shouldn't be important to know that it's a scoped_refptr. But either way is fine. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:677: class ResourceDispatcher { My suggestion is to not use IPC_BEGIN_MESSAGE_MAP at all and use a switch statement :) See content_renderer_pepper_host_factory.cc. I think my above suggestion will simplify this change significantly.
https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:124: file_system_type)); Done (comment). Actually, I think we really don't want to allow the plugin to send this message. We don't want to trust the root URL supplied by the plugin. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:66: content::PepperFileSystemHost* file_system_host = Fair enough, but I think it looks fine the way I currently have it. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:75: file_system_type); These are already being passed out of this function by reference. They will be created in-place in the pointer provided from CreateV8Value. CreateV8Value passes them by reference down to CreateResourceVarWithBrowserHost, which has to do a full copy of each of them to populate the HostResourceVar and vector, respectively. Avoiding that copy would be possible but introduce some annoying complexity (it isn't really conventional to have constructors accept scoped_ptrs). Basically each of these will be copied one time each, which I think is OK. The current message is very small (a few bytes). Eventually if we start embedding entire objects like blobs in messages, it would be appropriate to revisit avoiding any copy. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { On 2013/10/15 18:38:03, yzshen1 wrote: > More comment sounds like a good idea. :) > On 2013/10/15 00:25:16, Matt Giuca wrote: > > Yes, this was intentional (but perhaps not the best design). It's trickier > than > > that. See the comment here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... > > > > There are basically three outcomes: > > - Success. > > - Not a resource. > > - Unexpected error. > > > > The "not a resource" case cannot be considered an error. We have no way to > tell > > whether a V8 value is going to convert to a resource until we call this > method. > > If it's a resource, v8_var_converter will proceed to treat the V8 value as an > > ordinary dictionary. If it's an error, v8_var_converter will error out. This > > logic is already committed, and it's a pain to change it (if you want to > change > > it, it's outside the scope of this CL). > > > > I agree, it's not ideal to have the function return true but leave some inputs > > unchanged. A quick fix would be to simply set |result| to PP_MakeNull() in the > > "not a resource" case. Is that acceptable? > Done. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:123: *result = result_var->GetPPVar(); OK. (Also it would be a very long line, so I'll leave it.) https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/ppapi_messages.h#... ppapi/proxy/ppapi_messages.h:1303: PP_FileSystemType /* file_system_type */) On 2013/10/11 04:02:24, raymes wrote: > Could you fully document both of these? In particular state what action they > will trigger when they are received, which channel they are passed over and what > the parameters are for. Done. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:734: ResourceCreationProxy proxy(dispatcher_); It looks as though the ResourceCreationProxy is not used any more. GetConnection is still required, though, for the FileSystemResource. I don't know what EnterResourceCreationNoLock is for. Do we need it? https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { Hmm, I'm actually really confused about how this will behave in the in-process case. (I also don't know how to manually test the in-process case.) PpapiGlobals has to be either PluginGlobals or HostGlobals, right? Which one is it in the in-process case? If it's PluginGlobals, then all of our ResourceVars will be PluginResourceVars; if HostGlobals, then they will all be HostResourceVars. This seems as though it will be quite broken. I don't know if fixing this will be as simple as calling Dispatcher::IsPlugin instead of IsPluginGlobals. In fact, I suspect those two will agree with one another. Is there a way we can simply disable this functionality in the in-process case? Otherwise, I may need to fix a lot of bugs for the in-process case before committing this one. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:790: } I just realised that the recent changes we made to allow pp_resource 0 to be passed between will mean that we cannot assume pp_resource is nonzero on the host side, or take pp_resource 0 to mean "pending" on the plugin side. Will have to change the way this works.
https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/1/content/browser/renderer_host... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:124: file_system_type)); On 2013/10/17 09:17:58, Matt Giuca wrote: > Done (comment). > > Actually, I think we really don't want to allow the plugin to send this message. > We don't want to trust the root URL supplied by the plugin. Ah, good point! https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:75: file_system_type); On 2013/10/17 09:17:58, Matt Giuca wrote: > These are already being passed out of this function by reference. They will be > created in-place in the pointer provided from CreateV8Value. > > CreateV8Value passes them by reference down to CreateResourceVarWithBrowserHost, > which has to do a full copy of each of them to populate the HostResourceVar and > vector, respectively. Avoiding that copy would be possible but introduce some > annoying complexity (it isn't really conventional to have constructors accept > scoped_ptrs). > > Basically each of these will be copied one time each, which I think is OK. The > current message is very small (a few bytes). Eventually if we start embedding > entire objects like blobs in messages, it would be appropriate to revisit > avoiding any copy. Because of the interface you have here, you also copy them right here. You could alleviate that by having scoped_ptrs for out-params... in the param list: scoped_ptr<IPC::Message>* create_message then, here: *create_message = new PpapiPluginMsg_... There's also nothing wrong or unconventional about having a constructor accept a scoped_ptr. I would read it as "this takes ownership of the given object", which would be accurate in this case. That said, it's probably a really minor performance difference. So might not be worth the trouble. Definitely okay to leave it out for this CL. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { The Var proxies aren't "refactored" to use the in-process router. They're the old style. The in-process case doesn't use ppapi/proxy code, so I believe this code will only be used for an out-of-process plugin. So I think IsPluginGlobals() is safe here.
https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { So (I'm obviously going to have to run the in-process one myself before I'm sure about this), do you think that the resource vars created in the in-process mode will be host resource vars or plugin resource vars? I'm not sure what the implications are of either (since the plugin-side stuff was written assuming they would be PluginResourceVars, and the host-side stuff was written assuming they would be HostResourceVars).
You should get HostResourceVars since they're created by the HostVarTracker. Same way ArrayBuffer works. It should be easy to verify that, but I'm pretty confident. HTH On Oct 17, 2013 6:00 PM, <mgiuca@chromium.org> wrote: > > https://codereview.chromium.**org/26564009/diff/1/ppapi/** > proxy/raw_var_data.cc<https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc> > File ppapi/proxy/raw_var_data.cc (right): > > https://codereview.chromium.**org/26564009/diff/1/ppapi/** > proxy/raw_var_data.cc#**newcode779<https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#newcode779> > ppapi/proxy/raw_var_data.cc:**779: if > (PpapiGlobals::Get()->**IsPluginGlobals()) { > So (I'm obviously going to have to run the in-process one myself before > I'm sure about this), do you think that the resource vars created in the > in-process mode will be host resource vars or plugin resource vars? I'm > not sure what the implications are of either (since the plugin-side > stuff was written assuming they would be PluginResourceVars, and the > host-side stuff was written assuming they would be HostResourceVars). > > https://codereview.chromium.**org/26564009/<https://codereview.chromium.org/2... > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch... > . > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/18 00:19:45, dmichael wrote: > You should get HostResourceVars since they're created by the > HostVarTracker. Same way ArrayBuffer works. It should be easy to verify > that, but I'm pretty confident. HTH > On Oct 17, 2013 6:00 PM, <mailto:mgiuca@chromium.org> wrote: If that's true, do we have a reference counting problem? The HostResourceVars don't do reference counting because they assume that the Resource objects don't exist on the host side, only the plugin.
https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:75: file_system_type); Oh I see where the extra copy is here. I've fixed the one you mention using a scoped_ptr. The other copy is a bit more work and I'm not convinced it's worth the change. https://codereview.chromium.org/26564009/diff/1/content/renderer/pepper/resou... content/renderer/pepper/resource_converter.cc:77: *browser_host_create_message = On 2013/10/11 19:21:36, dmichael wrote: > (same suggestion for this message) Done. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { OK I just wrote browser tests with the help of Raymes. He suggested that I write a test so I can make sure it fails in the in-process case. Currently, it DOES NOT fail. I'll have a closer look at this on Monday. Sorry this CL is turning in to a bit of a mess :)
All comments are addressed. PTAL. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:677: class ResourceDispatcher { OK DONE! You were right, this is much simpler. Look at the delta between PS 7 and 8. :) https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:779: if (PpapiGlobals::Get()->IsPluginGlobals()) { Actually, ignore the previous comment. The in-process tests DO fail (if not disabled), but because the asynchronous nature of them causes the in-process case to hang forever. (It's the same for Array and Dictionary.) So we cannot adequately test the in-process case. Raymes suggested that the ResourceVar will not be used in an in-process case (since you can't use NaCl modules in-process), and therefore we do not need to worry about this. https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:790: } On 2013/10/17 09:17:58, Matt Giuca wrote: > I just realised that the recent changes we made to allow pp_resource 0 to be > passed between will mean that we cannot assume pp_resource is nonzero on the > host side, or take pp_resource 0 to mean "pending" on the plugin side. Will have > to change the way this works. Done.
palmer: Hi, adding you for security review on ppapi_messages.h. Note that of the two new messages, neither can be used to send messages from the plugin to the renderer or browser: - PpapiHostMsg_FileSystem_CreateFromRenderer has special logic to ensure that it comes from the renderer, not the plugin. - PpapiPluginMsg_FileSystem_CreateFromPendingHost is sent from the renderer to the plugin. It is embedded in the ResourceVar and interpreted by a special method in the plugin (ResourceRawVarData::CreatePPVar). Thanks.
It would also be good to have a test for ResourceConverter before this feature is complete. https://codereview.chromium.org/26564009/diff/251001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/251001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:110: nested_msg, &external_path)) { nit: I have a feeling this should be indented 4 spaces but I'm not certain (and below also) https://codereview.chromium.org/26564009/diff/251001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/251001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:71: if (!*pending_renderer_id) Maybe make this explicit... *pending_renderer_id != 0 https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:731: DCHECK(pp_resource_); I think this DCHECK might be a bit uneeded. This case shouldn't really happen. https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:595: ASSERT_TRUE(file_system_interface_->IsFileSystem(result)); Can we test some properties of the file system match? Could we write a file and compare the contents on each side? https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.h (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.h:87: // Test sending Resource vars in both directions. nit: This comment is inaccurate. Currently we only test JS->Plugin
https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:74: return Connection(PluginGlobals::Get()->GetBrowserSender(), dispatcher); I don't think this would do the right thing if it was called in-process. Maybe it would be good to document that or add a DCHECK? https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:716: case PpapiPluginMsg_FileSystem_CreateFromPendingHost::ID: { switch seems like an odd construct here. How about just an if/else? https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:719: *creation_message_, &file_system_type)) { nit: indent is one space too far
> It would also be good to have a test for ResourceConverter > before this feature is complete. Do you think it's necessary to have a specific test for ResourceConverter, now that we basically have a complete browser test for the whole thing? I'm thinking this will be complex to set up because it involves creating V8 values. If so, did you want me to do it in this CL, or before, or after? https://codereview.chromium.org/26564009/diff/251001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right): https://codereview.chromium.org/26564009/diff/251001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_renderer_connection.cc:110: nested_msg, &external_path)) { On 2013/10/22 05:59:21, raymes wrote: > nit: I have a feeling this should be indented 4 spaces but I'm not certain (and > below also) Done. https://codereview.chromium.org/26564009/diff/251001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/251001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:71: if (!*pending_renderer_id) On 2013/10/22 05:59:21, raymes wrote: > Maybe make this explicit... *pending_renderer_id != 0 Done. (But it's *pending_renderer_id == 0) https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:74: return Connection(PluginGlobals::Get()->GetBrowserSender(), dispatcher); On 2013/10/22 16:04:28, dmichael wrote: > I don't think this would do the right thing if it was called in-process. Maybe > it would be good to document that or add a DCHECK? Done. (We reasoned that GetForInstance will always return NULL in the in-process case.) https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:716: case PpapiPluginMsg_FileSystem_CreateFromPendingHost::ID: { Why? This is just the first type of resource we are going to allow passing -- in the future there may be many others. This code is taking an enum value, and performing a different action depending on which specific value it takes, with a default case. It is perfect for a switch. I realise that similar code in pepper_renderer_connection.cc also uses an if/else -- I think that should be a switch also. I'm happy to change that over to a switch for consistency. But if you really want these to be ifs, I can change this one. https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:719: *creation_message_, &file_system_type)) { ClangFormat did it. It's indenting four spaces from the start of the function name (not from the start of the expression), which I believe is correct. https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:731: DCHECK(pp_resource_); Well, the point of a DCHECK is to check for things that shouldn't really happen :). But in this case it is quite unlikely, so I removed it. https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:595: ASSERT_TRUE(file_system_interface_->IsFileSystem(result)); On 2013/10/22 05:59:21, raymes wrote: > Can we test some properties of the file system match? Could we write a file and > compare the contents on each side? Done. (I just want you to know how hard this was!) https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.h (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.h:87: // Test sending Resource vars in both directions. On 2013/10/22 05:59:21, raymes wrote: > nit: This comment is inaccurate. Currently we only test JS->Plugin Done.
lgtm https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/251001/ppapi/proxy/raw_var_data... ppapi/proxy/raw_var_data.cc:716: case PpapiPluginMsg_FileSystem_CreateFromPendingHost::ID: { Oh, right, switch/case is fine. I wasn't thinking about future resource types. https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:637: callback_type()); We usually just reuse the first TestCompletionCallback. GetCallback() returns a new pp::CompletionCallback each time.
Oops, lost one of my comments somehow. Trying again. https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:313: int TestPostMessage::WaitForMessagesAsync() { This function only works right if you've called PostMessageFromJavaScriptAsync exactly once before it. 0 will cause a hang, >1 will be dropped (and 1 is returned). What about rolling these two new methods in to one, that posts 1 message and then returns some indication as to whether that message was received. Then it will be harder to use incorrectly.
LGTM Thanks! https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:734: ResourceCreationProxy proxy(dispatcher_); ResourceCreationProxy is what we use for most resource creation at the plugin side. EnterResourceCreationNoLock.functions() points to a ResourceCreationProxy object. One way is to add a new CreateFileSystem() function signature to ResourceCreationAPI/Proxy, and then use EnterResourceCreationNoLock. The advantage is that you don't need to re-implement a function to get a connection. (Optional.) On 2013/10/17 09:17:58, Matt Giuca wrote: > It looks as though the ResourceCreationProxy is not used any more. GetConnection > is still required, though, for the FileSystemResource. > > I don't know what EnterResourceCreationNoLock is for. Do we need it? https://codereview.chromium.org/26564009/diff/511001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/511001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { nit, optional: it seems more clear to check the opposite "if (dom_file_system.isNull())" and return early if true.
https://codereview.chromium.org/26564009/diff/511001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/511001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:112: if (!dom_file_system.isNull()) { No, I don't want to do that, because this function is designed with the following structure: If can convert resource type A: Do it Else if can convert resource type B: Do it Else if can convert resource type C: Do it Else: Return true It just so happens that there is only one resource type to convert at the moment, so it looks a bit weird. But going to an "early exit" style will not allow us to add more resources in the future. https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:313: int TestPostMessage::WaitForMessagesAsync() { On 2013/10/23 17:36:02, dmichael wrote: > This function only works right if you've called PostMessageFromJavaScriptAsync > exactly once before it. 0 will cause a hang, >1 will be dropped (and 1 is > returned). > > What about rolling these two new methods in to one, that posts 1 message and > then returns some indication as to whether that message was received. Then it > will be harder to use incorrectly. Done. https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:637: callback_type()); On 2013/10/23 17:33:21, dmichael wrote: > We usually just reuse the first TestCompletionCallback. GetCallback() returns a > new pp::CompletionCallback each time. Done. (I copied the callback_1 style from test_file_io.cc; it's quite prevalent there.)
https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1/ppapi/proxy/raw_var_data.cc#n... ppapi/proxy/raw_var_data.cc:734: ResourceCreationProxy proxy(dispatcher_); OK, this seems like a complicated change (touching files which I am not otherwise touching in this CL). Maybe we can save that for post-commit cleanup.
Hi Tom, Chris told me he's too busy for a review. Would you be able to look at the ppapi_messages change?
Looking ... On Thu, Oct 24, 2013 at 3:56 PM, <mgiuca@chromium.org> wrote: > Hi Tom, > > Chris told me he's too busy for a review. Would you be able to look at the > ppapi_messages change? > > https://codereview.chromium.**org/26564009/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:1297: std::string /* root_url */, How does the browser know if the renderer has permission to create a file system at the specific root_url? It can't just trust that the renderer isn't lying.
https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:1297: std::string /* root_url */, I don't really know the answer to this. I thought that there was a security sandbox between the renderer and the browser, such that the browser would only let the renderer access files that it had explicitly opened. Or are we now subverting this sandbox by directly opening a file system in the browser? I don't fully understand the security model here. Adding teravest and kinuko who have a better understanding of file system stuff. Can one of you let me know if what I am doing here is OK from a security standpoint? In particular, look at this file and pepper_renderer_connection.cc.
https://codereview.chromium.org/26564009/diff/681001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/681001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:43: return PP_FILESYSTEMTYPE_EXTERNAL; In my understanding WebFileSystem::TypeExternal and PP_FILESYSTEM_EXTERNAL are not really same... am I wrong? PP_FILESYSTEMTYPE_EXTERNAL used to be for accessing non-sandboxed files with raw file path that was opened by file picker. https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/26564009/diff/681001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:1297: std::string /* root_url */, Arbitrarily creating a filesystem object with some root url should be relatively safe, or it'd provide the same level of security we do today for regular Web sites. Details are below: If the root_url points to sandbox filesystems (e.g. Temporary or Persistent) browser just allows the renderer (and its plugins) to access its own origin's filesystem. If renderer or plugin is compromised there's a chance that it may be able to access other origin's filesystem (but should never be able to access system files etc). If the root_url points to Isolated filesystem its filesystem ID is checked in the browser process so if the renderer (or its plugins) don't have permission to access the ID the succeeding requests should just fail. External filesystem in Pepper is a bit special but in general it's also protected by ChildProcessSecurityPolicy in the browser process (and this patch doesn't seem to enable it for external fs)
(still lgtm from my perspective, just an FYI comment) https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/511001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:637: callback_type()); On 2013/10/24 01:00:11, Matt Giuca wrote: > On 2013/10/23 17:33:21, dmichael wrote: > > We usually just reuse the first TestCompletionCallback. GetCallback() returns > a > > new pp::CompletionCallback each time. > > Done. (I copied the callback_1 style from test_file_io.cc; it's quite prevalent > there.) That's necessary when you have 2 operations in parallel and need 2 distinct callbacks at the same time. I think that's how FileIO uses it, FWIW.
lgtm with nits https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:302: // WaitForMessagesAsync correctly waits until the callback is called. This comment seems out of date now. https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:618: std::string file_path = "/"; nit: std::string file_path("/"); https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:622: ASSERT_EQ(kTestFilename, file_ref.GetName().AsString()); nit: Is this worth having? https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:627: callback_type()); nit: indentation
https://codereview.chromium.org/26564009/diff/681001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/26564009/diff/681001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:43: return PP_FILESYSTEMTYPE_EXTERNAL; I think they are the same. Look at this documentation: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... "Indicates a non-sandboxed filesystem." I don't exactly know what this is*, but it's clearly the case that the WebFileSystem::TypeExternal is the same thing as PP_FILESYSTEMTYPE_EXTERNAL. *I know that external file systems are used for chrome.syncFileSystem. Perhaps they are also used for non-sandboxed access to a real file system outside of NaCl. (I need to deal with external file systems in a follow-up CL, since they do not work here. So we can discuss this detail later.) https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:302: // WaitForMessagesAsync correctly waits until the callback is called. On 2013/10/27 21:59:34, raymes wrote: > This comment seems out of date now. Done. https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:618: std::string file_path = "/"; On 2013/10/27 21:59:34, raymes wrote: > nit: std::string file_path("/"); Done. https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:622: ASSERT_EQ(kTestFilename, file_ref.GetName().AsString()); On 2013/10/27 21:59:34, raymes wrote: > nit: Is this worth having? Done. (Fair enough; it doesn't test the API we're supposed to be testing.) https://codereview.chromium.org/26564009/diff/681001/ppapi/tests/test_post_me... ppapi/tests/test_post_message.cc:627: callback_type()); On 2013/10/27 21:59:34, raymes wrote: > nit: indentation Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/26564009/861001
Raymes, would you be able to look at Patch Set 16 to see if it makes sense. For others benefit: We ran into some deps issue because RawVarData is in the ppapi_ipc shared object, and it isn't allowed to talk to ppapi_proxy. So we decided the nicest way to deal with this would be to move the code from ResourceRawVarData::CreatePPVar into VarTracker.
Sorry, now look at the delta between PS 15 and 17. Thanks :)
https://codereview.chromium.org/26564009/diff/1041001/content/renderer/pepper... File content/renderer/pepper/host_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/content/renderer/pepper... content/renderer/pepper/host_var_tracker.cc:110: return VarTracker::MakeResourcePPVar(0); Should this be NOTREACHED? https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:33: // GetForInstance returns NULL in the in-process case. nit: PluginDispatcher::GetForInstance returns NULL... https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:196: return VarTracker::MakeResourcePPVar(pp_resource); nit: does this have to be qualified? Why not just call MakeResourceVar? https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.h (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.h:62: virtual PP_Var MakeResourcePPVar(const IPC::Message& creation_message, nit: I would put PP_Instance as the first param because it's more consistent with the rest of our code. https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/raw_var_dat... File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/raw_var_dat... ppapi/proxy/raw_var_data.cc:699: if (pp_resource_ || !creation_message_) nit: {} for multi-line if https://codereview.chromium.org/26564009/diff/1241001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1241001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:182: creation_message, &file_system_type)) { nit: indentation https://codereview.chromium.org/26564009/diff/1241001/ppapi/tests/test_post_m... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/1241001/ppapi/tests/test_post_m... ppapi/tests/test_post_message.cc:635: char* buffer = &buffer_vector.front(); // Note: Not null-terminated! nit &buffer[0] is more common.
lgtm
Yuzhu, David: Please take another look. I discussed this with Raymes. We agreed that in light of the refactor I had to do to fix the deps issues, one of you guys should have another look at this to make sure you agree with the VarTracker changes. Just look at the delta between Patch Set 15 and 19. I would appreciate a quick review -- I was hoping to submit this CL today as I will have more to do before the M32 branch point. I really hope I can submit it tomorrow. Thanks, Matt https://codereview.chromium.org/26564009/diff/1041001/content/renderer/pepper... File content/renderer/pepper/host_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/content/renderer/pepper... content/renderer/pepper/host_var_tracker.cc:110: return VarTracker::MakeResourcePPVar(0); Well, it's technically valid to call this -- on the host side, we always ignore creation_message. If the pp_resource is 0 and creation_message is not empty, we'll end up here, and we should just return a PP_Var with 0. I don't think we *will* ever get here, since why would creation_message be non-empty when deserializing a resource on the host side? But technically, this is allowed. https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:33: // GetForInstance returns NULL in the in-process case. On 2013/10/30 06:31:51, raymes wrote: > nit: PluginDispatcher::GetForInstance returns NULL... Done. https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:196: return VarTracker::MakeResourcePPVar(pp_resource); Something weird with the C++ compiler. Because the subclass overrides MakeResourceVar with 4 arguments, but not the 1-argument version, apparently if you just call MakeResourceVar from the subclass, it doesn't know about the 4-argument version. I had to qualify it. https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.h (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.h:62: virtual PP_Var MakeResourcePPVar(const IPC::Message& creation_message, On 2013/10/30 06:31:51, raymes wrote: > nit: I would put PP_Instance as the first param because it's more consistent > with the rest of our code. Done. https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/raw_var_dat... File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/raw_var_dat... ppapi/proxy/raw_var_data.cc:699: if (pp_resource_ || !creation_message_) On 2013/10/30 06:31:51, raymes wrote: > nit: {} for multi-line if Done. https://codereview.chromium.org/26564009/diff/1241001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1241001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:182: creation_message, &file_system_type)) { dmichael already picked this up. My reply: ClangFormat did it. It's indenting four spaces from the start of the function name (not from the start of the expression), which I believe is correct. https://codereview.chromium.org/26564009/diff/1241001/ppapi/tests/test_post_m... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/1241001/ppapi/tests/test_post_m... ppapi/tests/test_post_message.cc:635: char* buffer = &buffer_vector.front(); // Note: Not null-terminated! On 2013/10/30 06:31:51, raymes wrote: > nit &buffer[0] is more common. Done.
some nits, but o/w lgtm https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:196: return VarTracker::MakeResourcePPVar(pp_resource); On 2013/10/30 07:00:39, Matt Giuca wrote: > Something weird with the C++ compiler. Because the subclass overrides > MakeResourceVar with 4 arguments, but not the 1-argument version, apparently if > you just call MakeResourceVar from the subclass, it doesn't know about the > 4-argument version. I had to qualify it. It's called name hiding; the compiler is required to do this by the C++ standard. I would recommend avoiding the problem entirely by giving one of the functions a different name. (E.g., MakeResourcePPVarFromMessage or something). https://codereview.chromium.org/26564009/diff/1201002/content/renderer/pepper... File content/renderer/pepper/host_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1201002/content/renderer/pepper... content/renderer/pepper/host_var_tracker.cc:110: return VarTracker::MakeResourcePPVar(0); I *think* I have an idea what's going on, but it looks quite strange that you're ignoring all 4 parameters. It should probably be explained here why that is. https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:30: // Should only be called when the Pepper plugin is out of process. This comment isn't really necessary anymore, since PluginVarTracker is not used in-process. https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:33: // PluginDispatcher::GetForInstance returns NULL in the in-process case. Ditto. https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... ppapi/tests/test_post_message.cc:635: char* buffer = &buffer_vector[0]; // Note: Not null-terminated! I'm curious, what was wrong with the previous code?
I don't have suggestions other than those given by David. Thanks!
https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1041001/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:196: return VarTracker::MakeResourcePPVar(pp_resource); On 2013/10/30 17:47:27, dmichael wrote: > On 2013/10/30 07:00:39, Matt Giuca wrote: > > Something weird with the C++ compiler. Because the subclass overrides > > MakeResourceVar with 4 arguments, but not the 1-argument version, apparently > if > > you just call MakeResourceVar from the subclass, it doesn't know about the > > 4-argument version. I had to qualify it. > It's called name hiding; the compiler is required to do this by the C++ > standard. I would recommend avoiding the problem entirely by giving one of the > functions a different name. (E.g., MakeResourcePPVarFromMessage or something). Done. https://codereview.chromium.org/26564009/diff/1201002/content/renderer/pepper... File content/renderer/pepper/host_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1201002/content/renderer/pepper... content/renderer/pepper/host_var_tracker.cc:110: return VarTracker::MakeResourcePPVar(0); On 2013/10/30 17:47:27, dmichael wrote: > I *think* I have an idea what's going on, but it looks quite strange that you're > ignoring all 4 parameters. It should probably be explained here why that is. Done. https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:30: // Should only be called when the Pepper plugin is out of process. On 2013/10/30 17:47:27, dmichael wrote: > This comment isn't really necessary anymore, since PluginVarTracker is not used > in-process. Done. https://codereview.chromium.org/26564009/diff/1201002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:33: // PluginDispatcher::GetForInstance returns NULL in the in-process case. On 2013/10/30 17:47:27, dmichael wrote: > Ditto. Done. https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... ppapi/tests/test_post_message.cc:635: char* buffer = &buffer_vector[0]; // Note: Not null-terminated! You mean the one that read: int length = ...; char buffer[length]; ? MSVC didn't like it. Apparently, you aren't allowed to initialize a C-like array with a non-constant variable as the length. I'm not sure if this is a rule of C++ or a restriction in MSVC, but either way, this code is more likely to be standards compliant. (IIRC, the ability to do this was added formally in C99, but not C++. It's possibly in C++11, but we aren't allowed to use that yet.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/26564009/1511001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/26564009/1511001
Message was sent while issue was closed.
Change committed as 232080
Message was sent while issue was closed.
https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... File ppapi/tests/test_post_message.cc (right): https://codereview.chromium.org/26564009/diff/1201002/ppapi/tests/test_post_m... ppapi/tests/test_post_message.cc:635: char* buffer = &buffer_vector[0]; // Note: Not null-terminated! On 2013/10/30 23:13:54, Matt Giuca wrote: > You mean the one that read: > > int length = ...; > char buffer[length]; > > ? > > MSVC didn't like it. Apparently, you aren't allowed to initialize a C-like array > with a non-constant variable as the length. I'm not sure if this is a rule of > C++ or a restriction in MSVC, but either way, this code is more likely to be > standards compliant. > > (IIRC, the ability to do this was added formally in C99, but not C++. It's > possibly in C++11, but we aren't allowed to use that yet.) Oh, right, thanks for explaining. FYI, you could also use scoped_ptr<char[]> in this kind of situation since it's not going to need to grow. Not an important difference though here. |
