Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
173143005:
[PPAPI] It is now possible to pass a FileSystem to JS in PostMessage. (Closed)
|
Created:
6 years, 10 months ago by Matt Giuca Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, chrome-apps-syd-reviews_chromium.org, Ronghua Wu (Left Chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[PPAPI] It is now possible to pass a FileSystem to JS in PostMessage.
Upon receiving a resource, if it is a file system, the V8 Var converter
automatically converts it into a Blink DOMFileSystem and makes it
available in the V8 JavaScript context.
This also adds the possibility of converting other resource types to
JavaScript objects in the future.
BUG=345158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255891
Patch Set 1 #Patch Set 2 : Minor. #Patch Set 3 : Rebase off CL 173423004. #Patch Set 4 : Moved resource conversion logic from V8VarConverter to ResourceConverter. #
Total comments: 2
Patch Set 5 : Rebase onto CL 176263003 (fix DEPS for ParseFileSystemSchemeURL). #Patch Set 6 : Added error checking for file system type. #Patch Set 7 : Updated PostMessage test to check both directions. #Patch Set 8 : Update comment. #
Total comments: 20
Patch Set 9 : Address Kinuko's concerns and fix compile breakage due to Blink API changes. #
Total comments: 2
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/173143005/diff/130001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/130001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: *result_type = blink::WebFileSystemTypeExternal; As you can see in file_system_types.h, these are guaranteed to be the same value so that just static_cast should work. (If you want extra guarantee you can add static assertion)
NOT INTENDED FOR REVIEW (just responding to a comment). https://codereview.chromium.org/173143005/diff/130001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/130001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: *result_type = blink::WebFileSystemTypeExternal; I'm uncomfortable relying on that fact. Besides, I'm converting from FileSystemType (which is a superset of WebFileSystemType), not the other way around. We need a switch to ensure that it is one of these four types, and if not, it needs to be an error. (Note: I wasn't checking this error before, now I am.) For example, apparently ParseFileSystemSchemeURL will return a mount_type of kFileSystemTypeTest if it encounters a path starting with "/test". This should be an error because it is not convertible to a blink::WebFileSystemType.
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:62: bool FileApiFileSystemTypeToWebFileSystemType( See Kinuko's comment on Patch Set 4 (not yet resolved).
PS. Now intended for review :)
lgtm https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); missing namespace https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:140: blink::WebString::fromUTF8(root_url.spec())); I defer to kinuko for the creation of the dom filesystem (everything between lines 126-140).
looks good, two questions + one nit https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:62: bool FileApiFileSystemTypeToWebFileSystemType( On 2014/02/24 04:45:37, Matt Giuca wrote: > See Kinuko's comment on Patch Set 4 (not yet resolved). I'm fine with this code. https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: default: nit: do we want to put NOTREACHED() here? https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); Hmm.. does it mean we may not get the same .name values if we pass DOMFileSystem to pepper and then do the other way around, if the original DOMFileSystem.name field was not created by fileapi::GetFileSystemName()? In either way, fileapi::GetFileSystemName(origin, type) is more common way to create a name. (s/mount_type/type/) https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( Let me make sure once more (recalling past discussion)... we won't go through here across different origins/pages, right?
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( On 2014/02/25 05:07:05, kinuko wrote: > Let me make sure once more (recalling past discussion)... we won't go through > here across different origins/pages, right? Btw can you instead use new WebDOMFileSystem::create() (and toV8Value()) instead of WebFrame::createSerializableFileSystem? We're deprecating the WebFrame methods.
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: default: I don't think we want to. It isn't a programming error if the type is not supported, it could mean bad data from the plugin. This code is the first place where the file system type of the value passed from the user is checked. If the file type is some crazy type (which can happen if, for example, the URL ends with "/test"), then this is a user error, not a Chrome error, and it should fail properly, not crash the renderer. https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); On 2014/02/25 05:07:05, kinuko wrote: > Hmm.. does it mean we may not get the same .name values if we pass DOMFileSystem > to pepper and then do the other way around, if the original DOMFileSystem.name > field was not created by fileapi::GetFileSystemName()? I don't understand this concern. Can you elaborate? > In either way, fileapi::GetFileSystemName(origin, type) is more common way to > create a name. (s/mount_type/type/) Do you just mean I should rename my mount_type variable to type? (Done). Or am I using GetFileSystemName incorrectly? https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( On 2014/02/25 05:07:05, kinuko wrote: > Let me make sure once more (recalling past discussion)... we won't go through > here across different origins/pages, right? I don't know what you mean exactly. Can you explain the question in more detail? Do you mean "will the plugin have the same origin to the page it is embedded in?" (I believe the answer is yes.) > Btw can you instead use new WebDOMFileSystem::create() (and toV8Value()) instead > of WebFrame::createSerializableFileSystem? > > We're deprecating the WebFrame methods. Done.
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: default: On 2014/03/06 01:06:26, Matt Giuca wrote: > I don't think we want to. It isn't a programming error if the type is not > supported, it could mean bad data from the plugin. > > This code is the first place where the file system type of the value passed from > the user is checked. If the file type is some crazy type (which can happen if, > for example, the URL ends with "/test"), then this is a user error, not a Chrome > error, and it should fail properly, not crash the renderer. Ok (btw NOTREACHED won't crash renderer in production build) https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); On 2014/03/06 01:06:26, Matt Giuca wrote: > On 2014/02/25 05:07:05, kinuko wrote: > > Hmm.. does it mean we may not get the same .name values if we pass > DOMFileSystem > > to pepper and then do the other way around, if the original DOMFileSystem.name > > field was not created by fileapi::GetFileSystemName()? > > I don't understand this concern. Can you elaborate? I could be mistaken something. If we pass a DOMFileSystem [A] (whose name must have been initialized somewhere in chrome code when it's created) to plugin and then do the other way around it will go through this code, is my understanding correct? And then we create 'name' field here for the newly created DOMFileSystem, which may lead to have a different name from the original one ([A]), that's what I guessed. (I'm not telling that it's wrong or it should be avoided, just trying to understand what will happen) > > In either way, fileapi::GetFileSystemName(origin, type) is more common way to > > create a name. (s/mount_type/type/) > > Do you just mean I should rename my mount_type variable to type? (Done). Yes. Thanks. https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( On 2014/03/06 01:06:26, Matt Giuca wrote: > On 2014/02/25 05:07:05, kinuko wrote: > > Let me make sure once more (recalling past discussion)... we won't go through > > here across different origins/pages, right? > > I don't know what you mean exactly. Can you explain the question in more detail? > Do you mean "will the plugin have the same origin to the page it is embedded > in?" (I believe the answer is yes.) Yes, that's what I was asking. Serializable FileSystem isn't supposed to be passed across different origins, and I wanted to make sure it won't happen in your code. > > Btw can you instead use new WebDOMFileSystem::create() (and toV8Value()) > instead > > of WebFrame::createSerializableFileSystem? > > > > We're deprecating the WebFrame methods. > > Done.
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); On 2014/03/06 05:20:40, kinuko wrote: > On 2014/03/06 01:06:26, Matt Giuca wrote: > > On 2014/02/25 05:07:05, kinuko wrote: > > > Hmm.. does it mean we may not get the same .name values if we pass > > DOMFileSystem > > > to pepper and then do the other way around, if the original > DOMFileSystem.name > > > field was not created by fileapi::GetFileSystemName()? > > > > I don't understand this concern. Can you elaborate? > > I could be mistaken something. If we pass a DOMFileSystem [A] (whose name must > have been initialized somewhere in chrome code when it's created) to plugin and > then do the other way around it will go through this code, is my understanding > correct? > > And then we create 'name' field here for the newly created DOMFileSystem, which > may lead to have a different name from the original one ([A]), that's what I > guessed. (I'm not telling that it's wrong or it should be avoided, just trying > to understand what will happen) Probably this code's main target is to convert a pepper filesystem (that is created by PPAPI) into a corresponding DOMFileSystem? If so it'll likely be Temporary or Persistent filesystem, and in the case creating a name by GetFileSystemName() will give what's expected. So I assume this code is fine basically
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:78: default: Yeah, I know, but the debug build still shouldn't crash in response to a user error (if it's possible for a user to trigger a debug crash then that should be considered a bug). https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); OK thanks for explaining. You can also get other types of file system, basically from round-tripping. For example, you can pass an isolated file system from renderer to plugin and back. I have manually tested a round-trip of an isolated FS (my automated tests only test temporary FS roundtrip) and it seems to work OK. Why do you think the name would be different? I thought there was a standard pattern for naming file systems? https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( Yes, I think I remember this conversation now. I believe the plugin will always be in the same domain as the page. Raymes: Can you confirm this?
https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:132: std::string name = GetFileSystemName(origin, mount_type); On 2014/03/06 06:53:54, Matt Giuca wrote: > OK thanks for explaining. You can also get other types of file system, basically > from round-tripping. For example, you can pass an isolated file system from > renderer to plugin and back. > > I have manually tested a round-trip of an isolated FS (my automated tests only > test temporary FS roundtrip) and it seems to work OK. Why do you think the name > would be different? I thought there was a standard pattern for naming file > systems? Most (or possibly all) filesystems use the standard pattern, but technically each FileSystem backend can assign arbitrary name if it wants, since fileapi infra just delegates the task to each fs implementation, so we can't really guarantee it. I don't think it'll be a big problem in most cases, but wanted to note. https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:137: *dom_file_system = frame->createSerializableFileSystem( On 2014/03/06 06:53:54, Matt Giuca wrote: > Yes, I think I remember this conversation now. I believe the plugin will always > be in the same domain as the page. > > Raymes: Can you confirm this? Thanks. lgtm then (as far as Raymes says yes to this question)
If I understand your question correctly, I believe that the plugin can be from a different origin to the the page it is embedded it. This is simple to test (try embedding a random flash plugin in your own page). So in other words I think we night need to perform some security checks here (though I could be mistaken). It's a good catch kinuko! On Wed, Mar 5, 2014 at 11:05 PM, <kinuko@chromium.org> wrote: > > https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... > File content/renderer/pepper/resource_converter.cc (right): > > https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... > content/renderer/pepper/resource_converter.cc:132: std::string name = > GetFileSystemName(origin, mount_type); > On 2014/03/06 06:53:54, Matt Giuca wrote: >> >> OK thanks for explaining. You can also get other types of file system, > > basically >> >> from round-tripping. For example, you can pass an isolated file system > > from >> >> renderer to plugin and back. > > >> I have manually tested a round-trip of an isolated FS (my automated > > tests only >> >> test temporary FS roundtrip) and it seems to work OK. Why do you think > > the name >> >> would be different? I thought there was a standard pattern for naming > > file >> >> systems? > > > Most (or possibly all) filesystems use the standard pattern, but > technically each FileSystem backend can assign arbitrary name if it > wants, since fileapi infra just delegates the task to each fs > implementation, so we can't really guarantee it. I don't think it'll be > a big problem in most cases, but wanted to note. > > > https://codereview.chromium.org/173143005/diff/390001/content/renderer/pepper... > content/renderer/pepper/resource_converter.cc:137: *dom_file_system = > frame->createSerializableFileSystem( > On 2014/03/06 06:53:54, Matt Giuca wrote: >> >> Yes, I think I remember this conversation now. I believe the plugin > > will always >> >> be in the same domain as the page. > > >> Raymes: Can you confirm this? > > > Thanks. lgtm then (as far as Raymes says yes to this question) > > https://codereview.chromium.org/173143005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Discussed with Raymes. I'm going to take this particular discussion offline. We agreed it doesn't affect this CL, so committing. Thanks Kinuko, Raymes!
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/173143005/490001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg
Not a comment to your cl. Just a question I have in order to pass MediaStreamTrack from nacl to js. https://codereview.chromium.org/173143005/diff/490001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/490001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:136: blink::WebDOMFileSystem web_dom_file_system = blink::WebDOMFileSystem::create( Hi Matt, In the video track case, the pepper host (PepperMediaStreamVideoTrackHost) already owns a blink::WebMediaStreamTrack. I wonder do I need to associate that track with |frame|? Or can I just simple get the v8::Value like below? bool ResourceHostToDOMMediaStreamVideoTrack( content::PepperMediaStreamVideoTrackHost* host, v8::Handle<v8::Context> context, v8::Handle<v8::Value>* dom_video_track) { blink::WebMediaStreamTrack track = host->GetTrack(); *dom_video_track = track.toV8Value(); return true; }
https://codereview.chromium.org/173143005/diff/490001/content/renderer/pepper... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/173143005/diff/490001/content/renderer/pepper... content/renderer/pepper/resource_converter.cc:136: blink::WebDOMFileSystem web_dom_file_system = blink::WebDOMFileSystem::create( Since this isn't relevant to reviewing the CL, I'd prefer to discuss this offline.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/173143005/490001
Message was sent while issue was closed.
Change committed as 255891 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
