Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
250143002:
Media Galleries API: Audio/Video attached pictures support. (Closed)
|
Created:
6 years, 8 months ago by tommycli Modified:
6 years, 6 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, vandebo (ex-Chrome), Lei Zhang, tzik, nhiroki, Greg Billock, chromium-apps-reviews_chromium.org, kinuko+watch, michaeln Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMedia Galleries API: Audio/Video attached pictures support.
This patch enables audio / video thumbnail extraction. A lot of supporting code has already gone in. This is the last piece in the actual extension API that enables this.
BUG=304290
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274849
Patch Set 1 #Patch Set 2 : #
Total comments: 22
Patch Set 3 : upload some changes #Patch Set 4 : transition to AttachedPicture struct and eliminate a copy #
Total comments: 2
Patch Set 5 : change all 'picture' to 'image' #Patch Set 6 : Create Blobs on browser-process, eliminating two IPC copies. #
Total comments: 14
Patch Set 7 : make blob_storage_host a content/public interface with a content-private impl #
Total comments: 4
Patch Set 8 : #Patch Set 9 : remove a stray file #
Total comments: 1
Patch Set 10 : remove stray includes address comments #Patch Set 11 : #Patch Set 12 : remove stray changes. clarify some variable naming #
Total comments: 4
Patch Set 13 : Broke media/ portion off into its own patch. #Patch Set 14 : Spinoff the SafeMediaMetadataParser/Utility-process portion into its own patch. #Patch Set 15 : #Patch Set 16 : Rebase. #Patch Set 17 : Update to upstream #Patch Set 18 : #
Total comments: 3
Patch Set 19 : rebase #
Total comments: 8
Patch Set 20 : address vandebo comments #Patch Set 21 : #Patch Set 22 : remove the extra loop #Patch Set 23 : fix linux_chromium_chromeos_rel argument evaluation order bug #Messages
Total messages: 45 (0 generated)
vandebo: Let me know how you feel about the overall approach. I pretty much did it via the simplest way that could possibly work.
https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:873: // The custom JS binding needs the extra empty list parameters. Instead of using a list, use a dictionary. https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:904: // The custom JS binding will use the string vectors to create Blobs. Can't we create the blob backing store on the browser side? https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:269: void SniffMimeType(bool mime_type_only, Should this now be named GetMetadata ? https://codereview.chromium.org/250143002/diff/20001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:70: if (attached_pictures_bytes.size() != attached_pictures_types.size()) { nit: no {} Should this be a DCHECK ? https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:20: // Retrieve the mime type, metadata tags, and attached pictures. Is there any benefit (speed?) to retrieving the attached pictures [maybe images is a better word here] and not the tags? https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:153: // The pictures embedded in the media file's metadata. This is most often s/pictures/images/ https://codereview.chromium.org/250143002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js (right): https://codereview.chromium.org/250143002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js:120: chrome.test.assertEq(155752, metadata.attachedPictures[0].size); Maybe check a few bytes at the start and end as well? https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... File chrome/utility/media_galleries/media_metadata_parser.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... chrome/utility/media_galleries/media_metadata_parser.cc:56: std::vector<std::string>* attached_pictures_bytes, Maybe there should be a struct ThumbnailData { std::string type; std::string data; }; Then you can just deal with std::vector<ThumbnailData> ? https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... chrome/utility/media_galleries/media_metadata_parser.cc:117: MediaMetadataParser::MetadataCallback callback, Is this needed? Can you just bind to the callback instead? https://codereview.chromium.org/250143002/diff/20001/media/base/audio_video_m... File media/base/audio_video_metadata_extractor.cc (right): https://codereview.chromium.org/250143002/diff/20001/media/base/audio_video_m... media/base/audio_video_metadata_extractor.cc:131: std::string buffer( Looks like an extra copy... maybe pushback an empty string and then swap them.
This is what I have so far without the changeover to browser-process Blob creation. https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:873: // The custom JS binding needs the extra empty list parameters. On 2014/04/23 23:22:45, vandebo wrote: > Instead of using a list, use a dictionary. Done. https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:269: void SniffMimeType(bool mime_type_only, On 2014/04/23 23:22:45, vandebo wrote: > Should this now be named GetMetadata ? Done. https://codereview.chromium.org/250143002/diff/20001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:70: if (attached_pictures_bytes.size() != attached_pictures_types.size()) { On 2014/04/23 23:22:45, vandebo wrote: > nit: no {} Should this be a DCHECK ? Done. Moving to a struct eliminated the need for this check. https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:20: // Retrieve the mime type, metadata tags, and attached pictures. On 2014/04/23 23:22:45, vandebo wrote: > Is there any benefit (speed?) to retrieving the attached pictures [maybe images > is a better word here] and not the tags? Not sure, but I doubt it. At least in the FFMpeg code flow, to access the attached pictures, you would have already read-in the media file's headers (with its metadata). https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:153: // The pictures embedded in the media file's metadata. This is most often On 2014/04/23 23:22:45, vandebo wrote: > s/pictures/images/ Done. You mean just the comment, and not all the variable names right? (I used 'pictures' just because that's how ffmpeg refers to it. I have no objection to using images everywhere if that makes more sense to you) https://codereview.chromium.org/250143002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js (right): https://codereview.chromium.org/250143002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js:120: chrome.test.assertEq(155752, metadata.attachedPictures[0].size); On 2014/04/23 23:22:45, vandebo wrote: > Maybe check a few bytes at the start and end as well? Done. https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... File chrome/utility/media_galleries/media_metadata_parser.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... chrome/utility/media_galleries/media_metadata_parser.cc:56: std::vector<std::string>* attached_pictures_bytes, On 2014/04/23 23:22:45, vandebo wrote: > Maybe there should be a struct ThumbnailData { > std::string type; > std::string data; > }; > Then you can just deal with std::vector<ThumbnailData> ? Done. https://codereview.chromium.org/250143002/diff/20001/chrome/utility/media_gal... chrome/utility/media_galleries/media_metadata_parser.cc:117: MediaMetadataParser::MetadataCallback callback, On 2014/04/23 23:22:45, vandebo wrote: > Is this needed? Can you just bind to the callback instead? Needed so that the callback could base::Owned the data. The callback takes a reference rather than a pointer. https://codereview.chromium.org/250143002/diff/20001/media/base/audio_video_m... File media/base/audio_video_metadata_extractor.cc (right): https://codereview.chromium.org/250143002/diff/20001/media/base/audio_video_m... media/base/audio_video_metadata_extractor.cc:131: std::string buffer( On 2014/04/23 23:22:45, vandebo wrote: > Looks like an extra copy... maybe pushback an empty string and then swap them. Done.
https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:153: // The pictures embedded in the media file's metadata. This is most often On 2014/04/29 00:15:51, tommycli wrote: > On 2014/04/23 23:22:45, vandebo wrote: > > s/pictures/images/ > > Done. You mean just the comment, and not all the variable names right? (I used > 'pictures' just because that's how ffmpeg refers to it. I have no objection to > using images everywhere if that makes more sense to you) images is shorter and more general than pictures, but I care much more about the public interface. https://codereview.chromium.org/250143002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:20: // Retrieve the mime type, metadata tags, and attached pictures. nit: pictures -> images
https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:153: // The pictures embedded in the media file's metadata. This is most often On 2014/04/29 16:09:03, vandebo wrote: > On 2014/04/29 00:15:51, tommycli wrote: > > On 2014/04/23 23:22:45, vandebo wrote: > > > s/pictures/images/ > > > > Done. You mean just the comment, and not all the variable names right? (I used > > 'pictures' just because that's how ffmpeg refers to it. I have no objection to > > using images everywhere if that makes more sense to you) > > images is shorter and more general than pictures, but I care much more about the > public interface. Done. https://codereview.chromium.org/250143002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/media_galleries.idl (right): https://codereview.chromium.org/250143002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/media_galleries.idl:20: // Retrieve the mime type, metadata tags, and attached pictures. On 2014/04/29 16:09:03, vandebo wrote: > nit: pictures -> images Done.
michaeln: I am trying to create Blobs on the browser process and pass them to a renderer. This is because I have a ~200KB byte array in the browser process, and want to avoid copying it to the renderer, and then back again when I make it a Blob. Before I proceed further, I would like your feedback to see if the approach I'm taking seems legit to you. Patchset 5 -> Patchset 6 is the delta to do this. This also requires a Webkit patch: https://codereview.chromium.org/259423002/ See my below comments on the kinda janky stuff I had to do to make this work. Let me know what you think. https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:42: #include "content/browser/fileapi/blob_storage_host.h" This violates the include rules right now. I imagine I would have to move blob_storage_host.h to content/public, and then make a blob_storage_host_impl.cc/h? Does that sound legit to you? https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:937: // Here we notably use a UUID generated in the browser process instead of I generate a UUID on the browser process. Passes my browser test, but not sure if it's kosher. https://codereview.chromium.org/250143002/diff/100001/content/browser/fileapi... File content/browser/fileapi/fileapi_message_filter.h (right): https://codereview.chromium.org/250143002/diff/100001/content/browser/fileapi... content/browser/fileapi/fileapi_message_filter.h:84: BlobStorageHost* GetBlobStorageHost() const; Plumbing to get the blob storage host. https://codereview.chromium.org/250143002/diff/100001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/250143002/diff/100001/content/public/browser/... content/public/browser/render_process_host.h:221: virtual BlobStorageHost* GetBlobStorageHost() const = 0; Plumbing to get the BlobStorageHost. https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo... File extensions/renderer/blob_native_handler.cc (right): https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo... extensions/renderer/blob_native_handler.cc:38: // Need to decrement the refcount to compensate for the extra count left over This is also janky, but necessary to keep the refcount straight, since the browser process creates the Blob and needs to keep a ref to keep the Blob alive long enough for this method to 'catch' the ball.
michaeln: ping
sorry, i hadn't noticed this cl, looking...
We should loop jam in since it's involves the content api. I can see two new
classes in the content api to support this, and a new BrowserContext method to
get a ptr to the BlobContext.
class BlobHandle {
public:
virtual std::string uuid() = 0;
virtual ~BlobHandle() {}
protected:
BlobHandle();
};
// For use on the UI thread.
class BlobContext {
public:
typedef base::Callback<void(scoped_ptr<BlobHandle>)>
BlobCallback;
virtual void CreateMemoryBackedBlob(
const char* data, size_t length,
const BlobCallback& callback) = 0;
protected:
BlobContext();
virtual ~BlobContext();
};
class BrowserContext {
...
virtual BlobContext* GetBlobContext() = 0;
...
};
The first class is a wrapper around the existing webkit_blob::BlobDataHandle
class. The second a wrapper around (the very poorly named)
ChromeBlobStorageContext (that class can derive from this interface).
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:42:
#include "content/browser/fileapi/blob_storage_host.h"
> This violates the include rules right now. I imagine I would have to move
> blob_storage_host.h to content/public, and then make a
> blob_storage_host_impl.cc/h?
>
> Does that sound legit to you?
We need to expose an interface in the content api to do this, but it should be
an api to the per-profile BlobStorageContext class instead of the per-process
BlobStorageHost class to better match how we've exposed other components.
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:921: this,
attached_images),
Kinda unrelated to the blob handling specifically, but of |attached_image| is as
large as you say, it might be better to make fewer copies of that data in the
browser process. As coded here i think i see three copies: the caller has a
copy, each of these two Bind closures creates their own copy.
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:937: //
Here we notably use a UUID generated in the browser process instead of
On 2014/04/29 23:29:26, tommycli wrote:
> I generate a UUID on the browser process. Passes my browser test, but not sure
> if it's kosher.
This is perfectly halal.
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:945:
CHECK(blob_storage->StartBuildingBlob(uuid));
The BlobStorageContext::AddFinishedBlob(...) method is more intended to support
blob creation and lifetime management in the browser process.
// Useful for coining blobs from within the browser process. If the
// blob cannot be added due to memory consumption, returns NULL.
scoped_ptr<BlobDataHandle> AddFinishedBlob(const BlobData* blob_data);
I think the content api we expose should basically be a wrapper around that
method that's callable from the UI thread. A wrapper that i think would work for
your use case...
typedef base::Callback<void(scoped_ptr<BlobDataHandle>)>
BlobCallback;
void CreateMemoryBackedBlob(
const char* data, size_t length,
BlobCallback);
We can add others as needed like CreateFileBackedBlob(path).
https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):
https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:113:
image.blobUUID, image.type, image.size);
nit: indent by 2 more
https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo...
File extensions/renderer/blob_native_handler.cc (right):
https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo...
extensions/renderer/blob_native_handler.cc:38: // Need to decrement the refcount
to compensate for the extra count left over
On 2014/04/29 23:29:26, tommycli wrote:
> This is also janky, but necessary to keep the refcount straight, since the
> browser process creates the Blob and needs to keep a ref to keep the Blob
alive
> long enough for this method to 'catch' the ball.
This new case of creating a blob in the browser and sending it to the renderer
should use the pattern that's being used for IDB. That pattern involves an 'ack'
when the recipient has 'caught the ball' so the browser-side knows to drop its
handle. Here's the CL that shows how this is handled in IDB.
https://codereview.chromium.org/196473018/
The pattern goes like this:
- browser coins a blob and holds onto a scoped_ptr<BlobDataHandle>
- browser sends <uuid,type,size> to renderer
- renderer constructs blink::Blob (which establishes its own ref to uuid).
- renderer sends 'ack', got the blob
- browser deletes its BlobDataHandle
Additionally, if for any reason the renderer will never construct that
blink:Blob, the application has to delete BlobDataHandle. In cases like renderer
crashed, page unloaded prior to message getting that far, some other error in
processing prevented the blink::Blob from being coined.
Hi michaeln,
+cc jam
Thanks for the detailed recipie. I think this is all pretty workable. I will do
it in a separate CL since it seems to be fairly extensive.
I have a few questions:
1. I see that IndexedDB has its own message filter that can hold the BlobHandle.
In my use case, once the Extension call creates the blob, it gets destructed,
and can't stick around to hold the blob reference. Moreover, it'd be nice to tie
the lifetime of the browser-created blob to the renderer in case the ball is
never caught. Making a message filter seems to meet both those requirements. Do
you think I should tack on this functionality to FileAPIMessageFilter? Or maybe
a BlobMessageFilter?
2. The UUID generated in the browser process isn't exactly the same form as in
the renderer. It's all upper case, and is entirely random, whereas the Webkit
UUID generator is lower case and complies with UUID Version 4 (some chars are
fixed). Problem?
3. The 'ball catcher' in my use case is going to be blob_native_handler.cc. Any
issue with sending the ACK via ScriptContext->GetRenderView() ? Or do you have a
suggestion for a dedicated objects whose job is to send the ACK?
4. This is going to duplicate a lot of code with the IndexedDB approach. I
assume we'll eventually merge and unify the two right?
On 2014/05/01 21:27:35, michaeln wrote:
> We should loop jam in since it's involves the content api. I can see two new
> classes in the content api to support this, and a new BrowserContext method to
> get a ptr to the BlobContext.
>
> class BlobHandle {
> public:
> virtual std::string uuid() = 0;
> virtual ~BlobHandle() {}
> protected:
> BlobHandle();
> };
>
> // For use on the UI thread.
> class BlobContext {
> public:
> typedef base::Callback<void(scoped_ptr<BlobHandle>)>
> BlobCallback;
>
> virtual void CreateMemoryBackedBlob(
> const char* data, size_t length,
> const BlobCallback& callback) = 0;
>
> protected:
> BlobContext();
> virtual ~BlobContext();
> };
>
> class BrowserContext {
> ...
> virtual BlobContext* GetBlobContext() = 0;
> ...
> };
>
> The first class is a wrapper around the existing webkit_blob::BlobDataHandle
> class. The second a wrapper around (the very poorly named)
> ChromeBlobStorageContext (that class can derive from this interface).
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
> File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> (right):
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:42:
> #include "content/browser/fileapi/blob_storage_host.h"
> > This violates the include rules right now. I imagine I would have to move
> > blob_storage_host.h to content/public, and then make a
> > blob_storage_host_impl.cc/h?
> >
> > Does that sound legit to you?
>
> We need to expose an interface in the content api to do this, but it should be
> an api to the per-profile BlobStorageContext class instead of the per-process
> BlobStorageHost class to better match how we've exposed other components.
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:921:
this,
> attached_images),
> Kinda unrelated to the blob handling specifically, but of |attached_image| is
as
> large as you say, it might be better to make fewer copies of that data in the
> browser process. As coded here i think i see three copies: the caller has a
> copy, each of these two Bind closures creates their own copy.
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:937: //
> Here we notably use a UUID generated in the browser process instead of
> On 2014/04/29 23:29:26, tommycli wrote:
> > I generate a UUID on the browser process. Passes my browser test, but not
sure
> > if it's kosher.
>
> This is perfectly halal.
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio...
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:945:
> CHECK(blob_storage->StartBuildingBlob(uuid));
> The BlobStorageContext::AddFinishedBlob(...) method is more intended to
support
> blob creation and lifetime management in the browser process.
>
> // Useful for coining blobs from within the browser process. If the
> // blob cannot be added due to memory consumption, returns NULL.
> scoped_ptr<BlobDataHandle> AddFinishedBlob(const BlobData* blob_data);
>
> I think the content api we expose should basically be a wrapper around that
> method that's callable from the UI thread. A wrapper that i think would work
for
> your use case...
>
> typedef base::Callback<void(scoped_ptr<BlobDataHandle>)>
> BlobCallback;
>
> void CreateMemoryBackedBlob(
> const char* data, size_t length,
> BlobCallback);
>
> We can add others as needed like CreateFileBackedBlob(path).
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc...
> File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
> (right):
>
>
https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc...
> chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:113:
> image.blobUUID, image.type, image.size);
> nit: indent by 2 more
>
>
https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo...
> File extensions/renderer/blob_native_handler.cc (right):
>
>
https://codereview.chromium.org/250143002/diff/100001/extensions/renderer/blo...
> extensions/renderer/blob_native_handler.cc:38: // Need to decrement the
refcount
> to compensate for the extra count left over
> On 2014/04/29 23:29:26, tommycli wrote:
> > This is also janky, but necessary to keep the refcount straight, since the
> > browser process creates the Blob and needs to keep a ref to keep the Blob
> alive
> > long enough for this method to 'catch' the ball.
>
> This new case of creating a blob in the browser and sending it to the renderer
> should use the pattern that's being used for IDB. That pattern involves an
'ack'
> when the recipient has 'caught the ball' so the browser-side knows to drop its
> handle. Here's the CL that shows how this is handled in IDB.
>
> https://codereview.chromium.org/196473018/
>
> The pattern goes like this:
> - browser coins a blob and holds onto a scoped_ptr<BlobDataHandle>
> - browser sends <uuid,type,size> to renderer
> - renderer constructs blink::Blob (which establishes its own ref to uuid).
> - renderer sends 'ack', got the blob
> - browser deletes its BlobDataHandle
>
> Additionally, if for any reason the renderer will never construct that
> blink:Blob, the application has to delete BlobDataHandle. In cases like
renderer
> crashed, page unloaded prior to message getting that far, some other error in
> processing prevented the blink::Blob from being coined.
https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:931: render_view_host()->GetProcess()->GetBlobStorageHost(); RenderViewHost and RenderProcessHost are not thread safe, they can only be used on the UI thread https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:947: CHECK(blob_storage->FinishBuildingBlob(uuid, it->type)); since you only need these three methods, can you combine them into one and not expose BlobStorageHost at all. it's best to keep that as an internal implementation detail. you can have one method like CreateBlob(uuid, item, type).
On 2014/05/01 22:27:09, tommycli wrote:
> Hi michaeln,
>
> +cc jam
>
> Thanks for the detailed recipie. I think this is all pretty workable. I will
do
> it in a separate CL since it seems to be fairly extensive.
>
> I have a few questions:
>
> 1. I see that IndexedDB has its own message filter that can hold the
BlobHandle.
> In my use case, once the Extension call creates the blob, it gets destructed,
> and can't stick around to hold the blob reference. Moreover, it'd be nice to
tie
> the lifetime of the browser-created blob to the renderer in case the ball is
> never caught. Making a message filter seems to meet both those requirements.
Do
> you think I should tack on this functionality to FileAPIMessageFilter? Or
maybe
> a BlobMessageFilter?
I don't know enough about the extension system scaffolding to give you any
advice on where to stash these scoped_ptr values until an ack (or child process
death) has occurred? I think the scoped_ptr<BlobHandle> semantics are pretty
good. You create you, own it and are responsible for getting rid of it when no
longer need it. It's not enough to associate with the child process and forget
about it. At the time the ball is dropped, it should be deleted, your
application code is in the best position to know when that is. Child processes
can run for a long time, so leaking it for the duration of the child process is
not a good answer.
> 2. The UUID generated in the browser process isn't exactly the same form as in
> the renderer. It's all upper case, and is entirely random, whereas the Webkit
> UUID generator is lower case and complies with UUID Version 4 (some chars are
> fixed). Problem?
Not a problem, all that matters is that the string value is unique withing the
browser.
> 3. The 'ball catcher' in my use case is going to be blob_native_handler.cc.
Any
> issue with sending the ACK via ScriptContext->GetRenderView() ? Or do you have
a
> suggestion for a dedicated objects whose job is to send the ACK?
So long as you send it after the ball has been caught (or after you've
determined that the ball with never be caught), anywhere a message can be sent
will do. Since the msg type is specific each use case, I don't think there is
dedicated object for this.
sender->Send(new MyAckMessageType(blobidThatWasReceived));
> 4. This is going to duplicate a lot of code with the IndexedDB approach. I
> assume we'll eventually merge and unify the two right?
I don't see where there's much code duplication?
An extension function lives for the life of a function call, but it has access to lots of other context about where the call is coming from. An ExtensionFunction can access its RenderProcessHost: ext_function->render_view_host()->GetProcess(); That is a per-child-process object which derives from SupportsUserData. Data can be attached there to scope it to the life of the process (like a collection of BlobHandles created by the MediaGallery). I don't know enough about the extension system ipc plumbing to give you any advice on how to route your 'ack' messages back.
Publishing my changes for reference. This patch now depends on https://codereview.chromium.org/266373006/, which contains the Blob-passing changes that michaeln recommended. The diff between patchset 6 and latest shows how the new Blob-passing works. https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:904: // The custom JS binding will use the string vectors to create Blobs. On 2014/04/23 23:22:45, vandebo wrote: > Can't we create the blob backing store on the browser side? Done. https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:921: this, attached_images), On 2014/05/01 21:27:35, michaeln wrote: > Kinda unrelated to the blob handling specifically, but of |attached_image| is as > large as you say, it might be better to make fewer copies of that data in the > browser process. As coded here i think i see three copies: the caller has a > copy, each of these two Bind closures creates their own copy. Done. https://codereview.chromium.org/250143002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:945: CHECK(blob_storage->StartBuildingBlob(uuid)); On 2014/05/01 21:27:35, michaeln wrote: > The BlobStorageContext::AddFinishedBlob(...) method is more intended to support > blob creation and lifetime management in the browser process. > > // Useful for coining blobs from within the browser process. If the > // blob cannot be added due to memory consumption, returns NULL. > scoped_ptr<BlobDataHandle> AddFinishedBlob(const BlobData* blob_data); > > I think the content api we expose should basically be a wrapper around that > method that's callable from the UI thread. A wrapper that i think would work for > your use case... > > typedef base::Callback<void(scoped_ptr<BlobDataHandle>)> > BlobCallback; > > void CreateMemoryBackedBlob( > const char* data, size_t length, > BlobCallback); > > We can add others as needed like CreateFileBackedBlob(path). Done. https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js (right): https://codereview.chromium.org/250143002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:113: image.blobUUID, image.type, image.size); On 2014/05/01 21:27:35, michaeln wrote: > nit: indent by 2 more Done. https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:931: render_view_host()->GetProcess()->GetBlobStorageHost(); On 2014/05/02 18:31:59, jam wrote: > RenderViewHost and RenderProcessHost are not thread safe, they can only be used > on the UI thread Done. https://codereview.chromium.org/250143002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:947: CHECK(blob_storage->FinishBuildingBlob(uuid, it->type)); On 2014/05/02 18:31:59, jam wrote: > since you only need these three methods, can you combine them into one and not > expose BlobStorageHost at all. it's best to keep that as an internal > implementation detail. you can have one method like CreateBlob(uuid, item, > type). Done. https://codereview.chromium.org/250143002/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc (right): https://codereview.chromium.org/250143002/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:69: // We need to make a scoped copy of this vector since it will be destroyed I tried to eliminate all the copies, but could not eliminate this one. The IPC message was const, so I could not do a data-move.
https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:945: extensions::BlobHolder::CreateForWebContents(contents); If the holder already exists, is CreateForWebContents smart about not recreating it? https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:992: SendResponse(true); What happens if the page which has invoked this method unloads prior to this response reaching it? How is the response processed in that case and do those handles stuff into the holder on line 948 get removed?
https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:945: extensions::BlobHolder::CreateForWebContents(contents); On 2014/05/08 21:22:24, michaeln wrote: > If the holder already exists, is CreateForWebContents smart about not recreating > it? Correct. It does nothing if it already exists. https://codereview.chromium.org/250143002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:992: SendResponse(true); On 2014/05/08 21:22:24, michaeln wrote: > What happens if the page which has invoked this method unloads prior to this > response reaching it? How is the response processed in that case and do those > handles stuff into the holder on line 948 get removed? That's a good question. The current design of BlobHolder would leak until the WebContents gets destroyed. If that's not good enough, I could associate BlobHandles with RenderViewHosts, listen to the to the RenderViewDeleted event, and delete on that. ExtensionRendererState::RenderViewHostObserver is an example of that. What happens exactly when a page is unloaded? Is the WebContents deleted, or just the RVH?
> > What happens if the page which has invoked this method unloads prior to this
> > response reaching it? How is the response processed in that case and do
those
> > handles stuff into the holder on line 948 get removed?
>
> That's a good question. The current design of BlobHolder would leak until the
> WebContents gets destroyed.
>
> If that's not good enough, I could associate BlobHandles with RenderViewHosts,
> listen to the to the RenderViewDeleted event, and delete on that.
> ExtensionRendererState::RenderViewHostObserver is an example of that.
>
> What happens exactly when a page is unloaded? Is the WebContents deleted, or
> just the RVH?
I don't know those classes so well.
But here's another related question for you about extension api function call
handling. Suppose the message makes it there, the page is still up, is there
anything that would prevent native Take method defined in the other cl from
being called? A different way to ensure the 'ack' ipc message is sent back would
be to have it in the ipc handler, but called after having called script.
void onextemthodresponse(...) {
if (stillMakesSenseToCallScript()) {
callscript(resp);
}
sendAcksAboutBlobOwnershipBeingTransferred();
// if for any reason blobs weren't picked up (page gone, whatever), they'll
get freed up now
}
On 2014/05/08 23:14:06, michaeln wrote:
> > > What happens if the page which has invoked this method unloads prior to
this
> > > response reaching it? How is the response processed in that case and do
> those
> > > handles stuff into the holder on line 948 get removed?
> >
> > That's a good question. The current design of BlobHolder would leak until
the
> > WebContents gets destroyed.
> >
> > If that's not good enough, I could associate BlobHandles with
RenderViewHosts,
> > listen to the to the RenderViewDeleted event, and delete on that.
> > ExtensionRendererState::RenderViewHostObserver is an example of that.
> >
> > What happens exactly when a page is unloaded? Is the WebContents deleted, or
> > just the RVH?
>
> I don't know those classes so well.
>
> But here's another related question for you about extension api function call
> handling. Suppose the message makes it there, the page is still up, is there
> anything that would prevent native Take method defined in the other cl from
> being called? A different way to ensure the 'ack' ipc message is sent back
would
> be to have it in the ipc handler, but called after having called script.
>
> void onextemthodresponse(...) {
> if (stillMakesSenseToCallScript()) {
> callscript(resp);
> }
> sendAcksAboutBlobOwnershipBeingTransferred();
> // if for any reason blobs weren't picked up (page gone, whatever), they'll
> get freed up now
> }
The native handler should always run. The JS custom bindings are shipped with
Chrome.
The extension and app APIs rely on the custom bindings to function correctly. If
they don't function correctly, we don't even have a functional extension API,
and all bets are off.
> The native handler should always run. The JS custom bindings are shipped with
> Chrome.
>
> The extension and app APIs rely on the custom bindings to function correctly.
If
> they don't function correctly, we don't even have a functional extension API,
> and all bets are off.
ok, i see how the Take function is invoked prior to calling client callback on
line 118. I don't think that covers the case where the document has been closed
prior to the getMetadata response arriving an the function below not being
invoked. With routed messages, i think messages received after the object with
routeid has been delete are dropped on the floor.
Is this using routed messages ids and what class of object is the recipient of
that message the renderer?
108 function(name, request, response) {
109 if (response.attachedImagesBlobInfo) {
110 for (var i = 0; i < response.attachedImagesBlobInfo.length; i++) {
111 var blobInfo = response.attachedImagesBlobInfo[i];
112 var blob = blobNatives.TakeBrowserProcessBlob(
113 blobInfo.blobUUID, blobInfo.type, blobInfo.size);
114 response.metadata.attachedImages.push(blob);
115 }
116 }
117
118 if (request.callback)
119 request.callback(response.metadata);
120 }
On 2014/05/09 19:00:46, michaeln1 wrote:
> > The native handler should always run. The JS custom bindings are shipped
with
> > Chrome.
> >
> > The extension and app APIs rely on the custom bindings to function
correctly.
> If
> > they don't function correctly, we don't even have a functional extension
API,
> > and all bets are off.
>
> ok, i see how the Take function is invoked prior to calling client callback on
> line 118. I don't think that covers the case where the document has been
closed
> prior to the getMetadata response arriving an the function below not being
> invoked. With routed messages, i think messages received after the object with
> routeid has been delete are dropped on the floor.
>
> Is this using routed messages ids and what class of object is the recipient of
> that message the renderer?
>
> 108 function(name, request, response) {
> 109 if (response.attachedImagesBlobInfo) {
> 110 for (var i = 0; i < response.attachedImagesBlobInfo.length; i++) {
> 111 var blobInfo = response.attachedImagesBlobInfo[i];
> 112 var blob = blobNatives.TakeBrowserProcessBlob(
> 113 blobInfo.blobUUID, blobInfo.type, blobInfo.size);
> 114 response.metadata.attachedImages.push(blob);
> 115 }
> 116 }
> 117
> 118 if (request.callback)
> 119 request.callback(response.metadata);
> 120 }
Hey. Thanks.
The Extension response IPC in extension_messages.h
// The browser sends this message in response to all extension api calls. The
// response data (if any) is one of the base::Value subclasses, wrapped as the
// first element in a ListValue.
IPC_MESSAGE_ROUTED4(ExtensionMsg_Response,
int /* request_id */,
bool /* success */,
base::ListValue /* response wrapper (see comment above) */,
std::string /* error */)
The message is handled by src/extensions/renderer/extension_helper.h/cc.
ExtensionHelper is a RenderViewObserver, so it should have the same lifetime as
the RenderView. So if RenderView dies, the message is never received and dropped
on the floor, as you said.
I've modified the Blob patch (https://codereview.chromium.org/266373006/) to
scope the lifetime of the Blob to the RenderViewHost. Presumably, if the
RenderView dies and can't catch the message, the corresponding RenderViewHost
will also be deleted, and so the Blob will be collected. I'm not a 'content'
expert, is that true?
> Hey. Thanks. > > The Extension response IPC in extension_messages.h > > // The browser sends this message in response to all extension api calls. The > // response data (if any) is one of the base::Value subclasses, wrapped as the > // first element in a ListValue. > IPC_MESSAGE_ROUTED4(ExtensionMsg_Response, > int /* request_id */, > bool /* success */, > base::ListValue /* response wrapper (see comment above) */, > std::string /* error */) > > The message is handled by src/extensions/renderer/extension_helper.h/cc. > ExtensionHelper is a RenderViewObserver, so it should have the same lifetime as > the RenderView. So if RenderView dies, the message is never received and dropped > on the floor, as you said. > > I've modified the Blob patch (https://codereview.chromium.org/266373006/) to > scope the lifetime of the Blob to the RenderViewHost. Presumably, if the > RenderView dies and can't catch the message, the corresponding RenderViewHost > will also be deleted, and so the Blob will be collected. I'm not a 'content' > expert, is that true? RenderView<stuff> looks more tab specific then document specific. Looking at how the response messages are handled the go thru the Dispatcher class (which appears to be a one-per-process object in the renderer) and then further delegated to RequestSender. I think the ball can get dropped in two places... 1) ExtensionHelper is deleted by the time the response gets there (tab closed). 2) In RequestSender::HandleResponseHandleResponse when the tab has navigated to a new document. void RequestSender::HandleResponse(int request_id, bool success, const base::ListValue& response, const std::string& error) { linked_ptr<PendingRequest> request = RemoveRequest(request_id); if (!request.get()) { // This can happen if a context is destroyed while a request is in flight. ***** looks like this is the 2nd place where the ball can get dropped ***** return; } blink::WebScopedUserGesture gesture(request->token); request->source->OnResponseReceived( request->name, request_id, success, response, error); } Here's an idea. The Dispatcher currently is setup to receive messages as well, it receives 'control' messages instead of 'routed' messages. Use control messages for the response instead of routed messages so the response will be handled even of the tab was closed (taking care of 1). Modify Dispatcher::OnExtensionResponse method to send the 'AckIGotTheBlobs' messages in all cases, remove the 'ack' sending from the TakeMethod.
yoz: Can I get a weigh-in from you before I start making changes to the extensions IPC system? The goal is to create Blobs on the browser process and pass them to an extension on the renderer process. The difficulty is that the browser process needs to hold a reference to the Blob to keep it alive long enough for the renderer to 'catch' it. Once the renderer has caught it, it should send an IPC back to the browser process to release the reference. However, if the renderer fails to catch it due to the page navigating away, the script failing to execute, etc., the Blob reference must be released on the browser process to prevent a memory leak. The extension/ and chrome/browser/extensions/ code is here: https://codereview.chromium.org/280393003/ This patch depends on the above CL. The current implementation "works", but michaeln has found some holes.
On 2014/05/13 00:12:16, tommycli wrote: > yoz: Can I get a weigh-in from you before I start making changes to the > extensions IPC system? > > The goal is to create Blobs on the browser process and pass them to an extension > on the renderer process. > > The difficulty is that the browser process needs to hold a reference to the Blob > to keep it alive long enough for the renderer to 'catch' it. Once the renderer > has caught it, it should send an IPC back to the browser process to release the > reference. > > However, if the renderer fails to catch it due to the page navigating away, the > script failing to execute, etc., the Blob reference must be released on the > browser process to prevent a memory leak. > > The extension/ and chrome/browser/extensions/ code is here: > > https://codereview.chromium.org/280393003/ > > This patch depends on the above CL. The current implementation "works", but > michaeln has found some holes. I have some questions about things I don't understand: (0) Is the blob meant to be read and written, or is it just read? (1) How are Blobs (generally) meant to be used? The BlobHandle referred to in the above CL doesn't seem to exist yet. If we are passing handles between processes, this is something that the security team may want to review. (2) What's the plan if the renderer is destroyed before it receives the IPC? (3) Why do we need to pass ownership to the renderer? Is it possible to keep ownership of the Blob in the browser process (sharing a lifetime with the render host)?
On 2014/05/12 21:30:12, michaeln wrote: > > Hey. Thanks. > > > > The Extension response IPC in extension_messages.h > > > > // The browser sends this message in response to all extension api calls. The > > // response data (if any) is one of the base::Value subclasses, wrapped as the > > // first element in a ListValue. > > IPC_MESSAGE_ROUTED4(ExtensionMsg_Response, > > int /* request_id */, > > bool /* success */, > > base::ListValue /* response wrapper (see comment above) > */, > > std::string /* error */) > > > > The message is handled by src/extensions/renderer/extension_helper.h/cc. > > ExtensionHelper is a RenderViewObserver, so it should have the same lifetime > as > > the RenderView. So if RenderView dies, the message is never received and > dropped > > on the floor, as you said. > > > > I've modified the Blob patch (https://codereview.chromium.org/266373006/) to > > scope the lifetime of the Blob to the RenderViewHost. Presumably, if the > > RenderView dies and can't catch the message, the corresponding RenderViewHost > > will also be deleted, and so the Blob will be collected. I'm not a 'content' > > expert, is that true? > > RenderView<stuff> looks more tab specific then document specific. Looking at how > the response messages are handled the go thru the Dispatcher class (which > appears to be a one-per-process object in the renderer) and then further > delegated to RequestSender. I think the ball can get dropped in two places... > 1) ExtensionHelper is deleted by the time the response gets there (tab closed). > 2) In RequestSender::HandleResponseHandleResponse when the tab has navigated to > a new document. > > void RequestSender::HandleResponse(int request_id, > bool success, > const base::ListValue& response, > const std::string& error) { > linked_ptr<PendingRequest> request = RemoveRequest(request_id); > > if (!request.get()) { > // This can happen if a context is destroyed while a request is in flight. > ***** looks like this is the 2nd place where the ball can get dropped ***** > return; > } > > blink::WebScopedUserGesture gesture(request->token); > request->source->OnResponseReceived( > request->name, request_id, success, response, error); > } > > > Here's an idea. The Dispatcher currently is setup to receive messages as well, > it receives 'control' messages instead of 'routed' messages. Use control > messages for the response instead of routed messages so the response will be > handled even of the tab was closed (taking care of 1). Modify > Dispatcher::OnExtensionResponse method to send the 'AckIGotTheBlobs' messages in > all cases, remove the 'ack' sending from the TakeMethod. michaeln: I don't have enough knowledge to give you a full response here. I've been researching this... 1) ExtensionHelper is a RenderViewObserver so if that's deleted, the RenderViewHost is also deleted, so the BlobHolder is also deleted. No leak here, I believe. 2) The code path you mentioned in your second case happens if RequestSender::InvalidateSource is called and the pending request is deleted. This is a leak if the corresponding RenderView or RenderProcess is not also destroyed. I don't know enough about this, so I'll have to do more research. Regarding your suggested flow. It sounds like you recommend making the extension code 'aware' of Blobs. Maybe adding a vector of Blob UUIDs along with every Extension response. That sounds like it could work. I'd need yoz's buy-in on that since he owns the extension/ code.
On 2014/05/14 00:31:58, Yoyo Zhou wrote: > On 2014/05/13 00:12:16, tommycli wrote: > > yoz: Can I get a weigh-in from you before I start making changes to the > > extensions IPC system? > > > > The goal is to create Blobs on the browser process and pass them to an > extension > > on the renderer process. > > > > The difficulty is that the browser process needs to hold a reference to the > Blob > > to keep it alive long enough for the renderer to 'catch' it. Once the renderer > > has caught it, it should send an IPC back to the browser process to release > the > > reference. > > > > However, if the renderer fails to catch it due to the page navigating away, > the > > script failing to execute, etc., the Blob reference must be released on the > > browser process to prevent a memory leak. > > > > The extension/ and chrome/browser/extensions/ code is here: > > > > https://codereview.chromium.org/280393003/ > > > > This patch depends on the above CL. The current implementation "works", but > > michaeln has found some holes. > > I have some questions about things I don't understand: > > (0) Is the blob meant to be read and written, or is it just read? > (1) How are Blobs (generally) meant to be used? The BlobHandle referred to in > the above CL doesn't seem to exist yet. If we are passing handles between > processes, this is something that the security team may want to review. > (2) What's the plan if the renderer is destroyed before it receives the IPC? > (3) Why do we need to pass ownership to the renderer? Is it possible to keep > ownership of the Blob in the browser process (sharing a lifetime with the render > host)? (0). Just meant to be read right now. (1). BlobHandle doesn't exist right now. In https://codereview.chromium.org/266373006/. I can get a security review. (2). That is actually okay. The Blob is held in interim in the BlobHolder. (https://codereview.chromium.org/280393003/), which is scoped to the lifetime of the RenderView. (3). We want to return a Blob as a result of an ExtensionAPI call. We want the Blob's backing memory to be released when the last Javascript reference to it has been deleted. Everything would work fine if it shared the same lifetime as the RenderViewHost, it might just be seen as a little dirty.
I have one question. (4) What's the plan if the frame navigates to a new document before it receives the IPC?
On 2014/05/20 00:31:31, michaeln wrote: > I have one question. > > (4) What's the plan if the frame navigates to a new document before it receives > the IPC? (4) The Blob is likely leaked for the lifetime of the RenderViewHost. I did a quick test by adding DLOG(INFO) << "~RenderViewHostImpl()"; to the destructor of RenderViewHostImpl, and confirmed that navigation does not destroy the RenderViewHost. One thing we could do is to make BlobHolder listen to the WebContentsObserver notifications regarding navigation. We would store some kind of ID or pointer to the RenderViewHost along with the Blob, and when the RenderViewHost navigates, we destroy all associated Blobs that are still hanging around... yoz - any thoughts?
On 2014/05/20 00:48:54, tommycli wrote: > On 2014/05/20 00:31:31, michaeln wrote: > > I have one question. > > > > (4) What's the plan if the frame navigates to a new document before it > receives > > the IPC? > > (4) The Blob is likely leaked for the lifetime of the RenderViewHost. I did a > quick test by adding > DLOG(INFO) << "~RenderViewHostImpl()"; to the destructor of > RenderViewHostImpl, and confirmed that navigation does not destroy the > RenderViewHost. We probably could/should tighten that up. The duration of time the browser process has to "hold" the reference is fairly short, just until the response has been delivered or an attempt at delivery has failed. Not for the life of the page, the life of the page is too long and effectively a leak. I mentioned earlier a strategy aimed at accomplishing a lifetime tied to the response processing. Here it is again... "Here's an idea. The Dispatcher currently is setup to receive messages as well, it receives 'control' messages instead of 'routed' messages. Use control messages for the response instead of routed messages so the response will be handled even of the tab was closed (taking care of 1). Modify Dispatcher::OnExtensionResponse method to send the 'AckIGotTheBlobs' messages in all cases, remove the 'ack' sending from the TakeMethod." > One thing we could do is to make BlobHolder listen to the WebContentsObserver > notifications regarding navigation. We would store some kind of ID or pointer to > the RenderViewHost along with the Blob, and when the RenderViewHost navigates, > we destroy all associated Blobs that are still hanging around... > > yoz - any thoughts? fwiw, too many moving parts, too many dependencies on other notifications.
On 2014/05/20 01:33:43, michaeln wrote: > On 2014/05/20 00:48:54, tommycli wrote: > > On 2014/05/20 00:31:31, michaeln wrote: > > > I have one question. > > > > > > (4) What's the plan if the frame navigates to a new document before it > > receives > > > the IPC? > > > > (4) The Blob is likely leaked for the lifetime of the RenderViewHost. I did a > > quick test by adding > > DLOG(INFO) << "~RenderViewHostImpl()"; to the destructor of > > RenderViewHostImpl, and confirmed that navigation does not destroy the > > RenderViewHost. > > We probably could/should tighten that up. The duration of time the browser > process has to "hold" the reference is fairly short, just until the response has > been delivered or an attempt at delivery has failed. Not for the life of the > page, the life of the page is too long and effectively a leak. > > I mentioned earlier a strategy aimed at accomplishing a lifetime tied to the > response processing. Here it is again... > > "Here's an idea. The Dispatcher currently is setup to receive messages as well, > it receives 'control' messages instead of 'routed' messages. Use control > messages for the response instead of routed messages so the response will be > handled even of the tab was closed (taking care of 1). Modify > Dispatcher::OnExtensionResponse method to send the 'AckIGotTheBlobs' messages in > all cases, remove the 'ack' sending from the TakeMethod." > > > One thing we could do is to make BlobHolder listen to the WebContentsObserver > > notifications regarding navigation. We would store some kind of ID or pointer > to > > the RenderViewHost along with the Blob, and when the RenderViewHost navigates, > > we destroy all associated Blobs that are still hanging around... > > > > yoz - any thoughts? > > fwiw, too many moving parts, too many dependencies on other notifications. Hey michaeln, here was my response to that suggestion: """ Regarding your suggested flow. It sounds like you recommend making the extension code 'aware' of Blobs. Maybe adding a vector of Blob UUIDs along with every Extension response. That sounds like it could work. I'd need yoz's buy-in on that since he owns the extension/ code. """ I've thought about it some more: * Seems like it would require adding a "std::vector<std::string> blob_uuids" field to the ExtensionMsg_Response. Seems kind of wrong given that only one extension API call so far will pass over a Blob. Also would require plumbing the Blob UUIDs down to the message creation point in ExtensionFunctionDispatcher. * Or you might recommend digging into the base::ListValue field of ExtensionMsg_Response. That would be an 'easy' hack to make work but seems like the Dispatcher shouldn't dig into the response contents. * I'd like to get yoz's weigh-in on this matter, since he owns the extension/ code.
> """ > Regarding your suggested flow. It sounds like you recommend making the extension > code 'aware' of Blobs. Maybe adding a vector of Blob UUIDs along with every > Extension response. That sounds like it could work. I'd need yoz's buy-in on > that since he owns the extension/ code. > """ This particular chunk of extension code is blob aware now. I definitely don't want to over generalize, but I do want this particular kind of extension api response to get the blob lifetime stuff right. This use case will probably be used as an example for other cases where a blob is cons'd up in the browser and sent to the renderer. > * Seems like it would require adding a "std::vector<std::string> blob_uuids" > field to the ExtensionMsg_Response. Seems kind of wrong given that only one > extension API call so far will pass over a Blob. Also would require plumbing the > Blob UUIDs down to the message creation point in ExtensionFunctionDispatcher. > > * Or you might recommend digging into the base::ListValue field of > ExtensionMsg_Response. That would be an 'easy' hack to make work but seems like > the Dispatcher shouldn't dig into the response contents. Another option might be to define a new msg type... ExtensionMsg_PleaseSendBlobAcks(std::vector<std::string> uuids); ... that is not 'routed' and is handled directly by the Dispatcher class in the renderer. Upon receipt it sends an ack for each id, or a single ExtensionHostMsg_BlobAcks(std::vector<std::string> uuids). Usage goes like this, after an ExtensionMsg_Response containing blobs is sent, send the ExtensionMsg_PleaseSendBlobAcks seperately, and delete the handles in the browser process upon receipt of the acks. > * I'd like to get yoz's weigh-in on this matter, since he owns the extension/ > code. Me too.
On 2014/05/20 19:04:21, michaeln wrote: > > """ > > Regarding your suggested flow. It sounds like you recommend making the > extension > > code 'aware' of Blobs. Maybe adding a vector of Blob UUIDs along with every > > Extension response. That sounds like it could work. I'd need yoz's buy-in on > > that since he owns the extension/ code. > > """ > > This particular chunk of extension code is blob aware now. I definitely don't > want to over generalize, but I do want this particular kind of extension api > response to get the blob lifetime stuff right. This use case will probably be > used as an example for other cases where a blob is cons'd up in the browser and > sent to the renderer. > > > * Seems like it would require adding a "std::vector<std::string> blob_uuids" > > field to the ExtensionMsg_Response. Seems kind of wrong given that only one > > extension API call so far will pass over a Blob. Also would require plumbing > the > > Blob UUIDs down to the message creation point in ExtensionFunctionDispatcher. > > > > * Or you might recommend digging into the base::ListValue field of > > ExtensionMsg_Response. That would be an 'easy' hack to make work but seems > like > > the Dispatcher shouldn't dig into the response contents. > > Another option might be to define a new msg type... > > ExtensionMsg_PleaseSendBlobAcks(std::vector<std::string> uuids); > > ... that is not 'routed' and is handled directly by the Dispatcher class in the > renderer. Upon receipt it sends an ack for each id, or a single > ExtensionHostMsg_BlobAcks(std::vector<std::string> uuids). > > Usage goes like this, after an ExtensionMsg_Response containing blobs is sent, > send the ExtensionMsg_PleaseSendBlobAcks seperately, and delete the handles in > the browser process upon receipt of the acks. > > > * I'd like to get yoz's weigh-in on this matter, since he owns the extension/ > > code. > > Me too. Disclaimer: I haven't reviewed serious extension IPC changes before. It sounds like the main issue is that the blob's lifetime in the renderer should be scoped to the main frame. I'm not sure if we have idioms for this already. I agree with Michael here: "Use control messages for the response instead of routed messages so the response will be handled even of the tab was closed." It seems like we'd need only 2 paths to delete the blob in the browser: (1) renderer is destroyed, (2) renderer (Dispatcher) sends an ack. I'm not sure we need the ExtensionMsg_PleaseSendBlobAcks though; could the renderer just send a ExtensionHostMsg_BlobAcks?
> Disclaimer: I haven't reviewed serious extension IPC changes before. i dont think this should amount to a serious change, i would hope not. > It sounds like the main issue is that the blob's lifetime in the renderer should > be scoped to the main frame. I'm not sure if we have idioms for this already. actually, the browser side need only hold the handle for the duration of the response processing in the renderer (at which time the renderer takes responsibility for eventually cleaning up). > I agree with Michael here: "Use control messages for the response instead of > routed messages so the response will be handled even of the tab was closed." It > seems like we'd need only 2 paths to delete the blob in the browser: (1) > renderer is destroyed, (2) renderer (Dispatcher) sends an ack. bingo! two code paths 1) the normal one for response is processed 2) the exceptional one for process dies > I'm not sure we need the ExtensionMsg_PleaseSendBlobAcks though; could the > renderer just send a ExtensionHostMsg_BlobAcks? so long as know what ids are being acked, i'm not sure if its easier to modify the existing 'Resposne' message types/processing, or to add the new 'please' message too... either way works.
On 2014/05/21 01:31:56, michaeln wrote: > > Disclaimer: I haven't reviewed serious extension IPC changes before. > > i dont think this should amount to a serious change, i would hope not. > > > It sounds like the main issue is that the blob's lifetime in the renderer > should > > be scoped to the main frame. I'm not sure if we have idioms for this already. > > actually, the browser side need only hold the handle for the duration of the > response processing in the renderer (at which time the renderer takes > responsibility for eventually cleaning up). > > > I agree with Michael here: "Use control messages for the response instead of > > routed messages so the response will be handled even of the tab was closed." > It > > seems like we'd need only 2 paths to delete the blob in the browser: (1) > > renderer is destroyed, (2) renderer (Dispatcher) sends an ack. > > bingo! two code paths 1) the normal one for response is processed 2) the > exceptional one for process dies > > > I'm not sure we need the ExtensionMsg_PleaseSendBlobAcks though; could the > > renderer just send a ExtensionHostMsg_BlobAcks? > > so long as know what ids are being acked, i'm not sure if its easier to modify > the existing 'Resposne' message types/processing, or to add the new 'please' > message too... either way works. michaeln: I've rebased this patch to onto the changes we discussed in this thread. The new TransferBlobAck mechanism is at https://codereview.chromium.org/280393003/. Thanks
vandebo: The last Blob transfer patch for etxensionsis currently in CQ. See https://codereview.chromium.org/280393003/ This is the patch that actually enables the thumbnails, PTAL. Thanks! https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:839: bool get_attached_images = Should the default really be to get the attached images? Not totally sure on this... it seems like the overhead of getting another 100KB over IPC must be small compared to process startup...
Also, update the description. https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:839: bool get_attached_images = On 2014/06/02 17:19:06, tommycli wrote: > Should the default really be to get the attached images? Not totally sure on > this... it seems like the overhead of getting another 100KB over IPC must be > small compared to process startup... The IDL implies that we only get images for type ALL. If you think the costs don't fall out that way, we should measure the costs for the different parts and only provide the options that save time (or CPU time == power). https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:849: bool mime_type_only, bool get_attached_images, You should probably make an enum here instead of passing around multiple bools. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:931: scoped_ptr<content::BlobHandle> next_blob) { This is the current or previous blob, not the next blob. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:967: void MediaGalleriesGetMetadataFunction::FinishRequest( It seems like you could do all of this work in either OnSafeMediaMetadataParserDone or ConstructNextBlob. Not sure if that would make them "too long" though https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:975: base::DictionaryValue* result_dictionary = new base::DictionaryValue; Why not do this in OnSafeMediaMetadataParserDone and pass it around instead of metadata_dictionary ?
https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/360001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:839: bool get_attached_images = On 2014/06/02 18:07:27, vandebo wrote: > On 2014/06/02 17:19:06, tommycli wrote: > > Should the default really be to get the attached images? Not totally sure on > > this... it seems like the overhead of getting another 100KB over IPC must be > > small compared to process startup... > > The IDL implies that we only get images for type ALL. If you think the costs > don't fall out that way, we should measure the costs for the different parts and > only provide the options that save time (or CPU time == power). Done. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:849: bool mime_type_only, bool get_attached_images, On 2014/06/02 18:07:27, vandebo wrote: > You should probably make an enum here instead of passing around multiple bools. Done. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:931: scoped_ptr<content::BlobHandle> next_blob) { On 2014/06/02 18:07:27, vandebo wrote: > This is the current or previous blob, not the next blob. Done. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:967: void MediaGalleriesGetMetadataFunction::FinishRequest( On 2014/06/02 18:07:27, vandebo wrote: > It seems like you could do all of this work in either > OnSafeMediaMetadataParserDone or ConstructNextBlob. Not sure if that would make > them "too long" though Done. https://codereview.chromium.org/250143002/diff/380001/chrome/browser/extensio... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:975: base::DictionaryValue* result_dictionary = new base::DictionaryValue; On 2014/06/02 18:07:27, vandebo wrote: > Why not do this in OnSafeMediaMetadataParserDone and pass it around instead of > metadata_dictionary ? Done.
vandebo: I removed the extra loop at the end that created the ListValue and instead made it part of the recursive call. +cc thestig in case he's interested.
blob lifetime stuff l g t m, i'll remove myself as reviewer for the media gallery specific'ness
LGTM
yoz: extensions/browser/blob_holder.cc has a small bugfix that needs your review. This can also be a separate patch if you prefer.
On 2014/06/03 18:18:13, tommycli wrote: > yoz: extensions/browser/blob_holder.cc has a small bugfix that needs your > review. This can also be a separate patch if you prefer. blob_holder.cc lgtm
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/250143002/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
Message was sent while issue was closed.
Change committed as 274849 |
