Blobs: Catching browser-process created Blobs in extension code.

Blobs: Catching browser-process created Blobs in extension code.

This is a spinoff of https://codereview.chromium.org/266373006/, Patchset 11.

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

[tommycli, 2014-05-19 16:12:31]

yoz: ping.

This is the spinoff patch of https://codereview.chromium.org/250143002/

Thanks!

[Yoyo Zhou, 2014-05-19 22:23:48]

LGTM

https://chromiumcodereview.appspot.com/280393003/diff/20001/extensions/render...
File extensions/renderer/blob_native_handler.cc (right):

https://chromiumcodereview.appspot.com/280393003/diff/20001/extensions/render...
extensions/renderer/blob_native_handler.cc:25: // Take ownership of a Blob
created on the browse process. Expects the Blob's
browser

https://chromiumcodereview.appspot.com/280393003/diff/20001/extensions/render...
extensions/renderer/blob_native_handler.cc:26: // UUID, type, and size as
arguments.
Document the return value.

[tommycli, 2014-05-19 23:02:09]

https://codereview.chromium.org/280393003/diff/20001/extensions/renderer/blob...
File extensions/renderer/blob_native_handler.cc (right):

https://codereview.chromium.org/280393003/diff/20001/extensions/renderer/blob...
extensions/renderer/blob_native_handler.cc:25: // Take ownership of a Blob
created on the browse process. Expects the Blob's
On 2014/05/19 22:23:48, Yoyo Zhou wrote:
> browser

Done.

https://codereview.chromium.org/280393003/diff/20001/extensions/renderer/blob...
extensions/renderer/blob_native_handler.cc:26: // UUID, type, and size as
arguments.
On 2014/05/19 22:23:48, Yoyo Zhou wrote:
> Document the return value.

Done.

[tommycli, 2014-05-19 23:03:57]

tsepez: Need a security review of extension_messages.h.

Also need a review of the general concept of creating Blobs in the browser
process and passing ownership over IPC to an extension in the renderer process.

See:
https://codereview.chromium.org/266373006/
https://codereview.chromium.org/250143002

Thanks!

[Tom Sepez, 2014-05-19 23:41:37]

The messages themselves seem OK, but can you fill me in a bit more on how you
intend to create the blobs?  I didn't see any code that actually created and
added blobs to your holders.

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:32: held_blobs_.push_back(new
BlobHolderData(render_view_host, blob.release()));
How do we ensure the UUID isn't duplicated?

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:66: DLOG(ERROR) << "Tried to release a
Blob we don't have ownership to. UUID: "
probably want to kill the renderer here via BadMessageReceived().

[tommycli, 2014-05-20 00:04:10]

tsepez:

Sure. This is the first patch that will use the new capabilities:
https://codereview.chromium.org/250143002

Lines 907-965 here:
https://codereview.chromium.org/250143002/diff/300001/chrome/browser/extensio...

The Blobs are created in BrowserContext::CreateMemoryBackedBlob, and then held
in the BlobHolder defined in this patch.

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:32: held_blobs_.push_back(new
BlobHolderData(render_view_host, blob.release()));
On 2014/05/19 23:41:37, Tom Sepez wrote:
> How do we ensure the UUID isn't duplicated?

Currently we don't. We just assumed that they're unique. Do you think we should
iterate through our existing held UUIDs here with a function within a DCHECK?

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:66: DLOG(ERROR) << "Tried to release a
Blob we don't have ownership to. UUID: "
On 2014/05/19 23:41:37, Tom Sepez wrote:
> probably want to kill the renderer here via BadMessageReceived().

Since this isn't a BrowserMessageFilter, I don't have access to that method.

I could do a web_contents()->Close() or a
web_contents()->GetRenderViewHost()->ClosePage(). Do either of those make sense?

[Tom Sepez, 2014-05-20 16:22:51]

On 2014/05/20 00:04:10, tommycli wrote:
> tsepez:
> 
> Sure. This is the first patch that will use the new capabilities:
> https://codereview.chromium.org/250143002
> 
> Lines 907-965 here:
>
https://codereview.chromium.org/250143002/diff/300001/chrome/browser/extensio...
> 
> The Blobs are created in BrowserContext::CreateMemoryBackedBlob, and then held
> in the BlobHolder defined in this patch.
> 
>
https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/blob_holder.cc (right):
> 
>
https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/blob_holder.cc:32: held_blobs_.push_back(new
> BlobHolderData(render_view_host, blob.release()));
> On 2014/05/19 23:41:37, Tom Sepez wrote:
> > How do we ensure the UUID isn't duplicated?
> 
> Currently we don't. We just assumed that they're unique. Do you think we
should
> iterate through our existing held UUIDs here with a function within a DCHECK?
> 
Sounds reasonable.

>
https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/blob_holder.cc:66: DLOG(ERROR) << "Tried to release
a
> Blob we don't have ownership to. UUID: "
> On 2014/05/19 23:41:37, Tom Sepez wrote:
> > probably want to kill the renderer here via BadMessageReceived().
> 
> Since this isn't a BrowserMessageFilter, I don't have access to that method.
> 
> I could do a web_contents()->Close() or a
> web_contents()->GetRenderViewHost()->ClosePage(). Do either of those make
sense?
Not really.  There's got to be a way to get the message filter, I would hope.

[tommycli, 2014-05-20 18:22:42]

tsepez: addressed your comments below

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:32: held_blobs_.push_back(new
BlobHolderData(render_view_host, blob.release()));
On 2014/05/20 00:04:11, tommycli wrote:
> On 2014/05/19 23:41:37, Tom Sepez wrote:
> > How do we ensure the UUID isn't duplicated?
> 
> Currently we don't. We just assumed that they're unique. Do you think we
should
> iterate through our existing held UUIDs here with a function within a DCHECK?

Done.

https://codereview.chromium.org/280393003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:66: DLOG(ERROR) << "Tried to release a
Blob we don't have ownership to. UUID: "
On 2014/05/20 00:04:11, tommycli wrote:
> On 2014/05/19 23:41:37, Tom Sepez wrote:
> > probably want to kill the renderer here via BadMessageReceived().
> 
> Since this isn't a BrowserMessageFilter, I don't have access to that method.
> 
> I could do a web_contents()->Close() or a
> web_contents()->GetRenderViewHost()->ClosePage(). Do either of those make
sense?

Done.

[Tom Sepez, 2014-05-20 18:24:23]

lgtm

[michaeln, 2014-05-20 19:08:19]

https://codereview.chromium.org/280393003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:33:
DCHECK(!ContainsUUID(blob->GetUUID()));
wait, this isn't correct. there can be multiple handles referring to the same
blob, those will have the same uuid. dups here are not indicative of an error.

You should never see the same BlobHandle* here.

[tommycli, 2014-05-20 19:49:05]

michaeln: Fixed the duplicate UUID thing.

Also see patchset 7 for what it might look like if we monitor navigations from
the BlobHolder.

The change I made only catches top-level navs, so I imagine if a script is
running in an iframe, and the iframe then navigates, we could still have a
problem.

To catch the IFrames, we'd have to key Blobs to RenderFrameHost instead of
RenderViewHost. That's blocked until extension dispatches pass in
RenderFrameHost. See
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

Anyways, I'm not married to this solution... just a possibility.

https://codereview.chromium.org/280393003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/blob_holder.cc:33:
DCHECK(!ContainsUUID(blob->GetUUID()));
On 2014/05/20 19:08:19, michaeln wrote:
> wait, this isn't correct. there can be multiple handles referring to the same
> blob, those will have the same uuid. dups here are not indicative of an error.
> 
> You should never see the same BlobHandle* here.

Done.

[tommycli, 2014-05-22 17:03:17]

michaeln: This patch uses control messages to guarantee Blob release. Let me
know what you think, and thanks for sticking with this.

[michaeln, 2014-05-23 00:12:34]

renderer side looks pretty good

https://codereview.chromium.org/280393003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/blob_holder.h (right):

https://codereview.chromium.org/280393003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/blob_holder.h:22: public
content::WebContentsUserData<BlobHolder> {
Since WebContents can outlive/span processes, i think the BlobHandleVector
should be scoped to the RenderProcessHost. RPH derives from SupportsUserData so
this collection of blobhandles in transite to that process could hang there as
UserData.

For processing the AckMessage in the browser process, ExtensionMessageFilter
looks like it may be a good place for that given the process specific'ness
involved. The handler in that filter class could get the RPH and tell the
attached holder to ReleaseBlobReference(uuid).

wdyt?

https://codereview.chromium.org/280393003/diff/200001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/280393003/diff/200001/extensions/browser/exte...
extensions/browser/extension_function.cc:357: GetRoutingID(),
transferred_blob_uuids_));
What's the purpose if the routeid here?

[tommycli, 2014-05-23 16:27:44]

michaeln: Addressed your comments below. I think we're very close. I'm on
vacation all next week, so if we could get this in CQ by end of day, that would
be amaze.

https://codereview.chromium.org/280393003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/blob_holder.h (right):

https://codereview.chromium.org/280393003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/blob_holder.h:22: public
content::WebContentsUserData<BlobHolder> {
On 2014/05/23 00:12:34, michaeln wrote:
> Since WebContents can outlive/span processes, i think the BlobHandleVector
> should be scoped to the RenderProcessHost. RPH derives from SupportsUserData
so
> this collection of blobhandles in transite to that process could hang there as
> UserData.
> 
> For processing the AckMessage in the browser process, ExtensionMessageFilter
> looks like it may be a good place for that given the process specific'ness
> involved. The handler in that filter class could get the RPH and tell the
> attached holder to ReleaseBlobReference(uuid).
> 
> wdyt?

Actually that makes a lot of sense. I basically made those changes verbatim.

https://codereview.chromium.org/280393003/diff/200001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/280393003/diff/200001/extensions/browser/exte...
extensions/browser/extension_function.cc:357: GetRoutingID(),
transferred_blob_uuids_));
On 2014/05/23 00:12:34, michaeln wrote:
> What's the purpose if the routeid here?

Previously needed it due to it being a WebContentsObserver. Now that the message
is handled in the ExtensionMessageFilter, this is no longer needed.

[tommycli, 2014-05-23 16:31:25]

yoz: Do you have any comments regarding the extensions/ changes that have
occurred since your review?

[michaeln, 2014-05-23 17:31:35]

both sides lgtm,

i have a couple comments but i'll leave it to you and yoz to decide upon

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
File extensions/browser/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
extensions/browser/blob_holder.cc:42: DCHECK(!ContainsBlobHandle(blob.get()));
might be able to simplify

DCHECK_EQ(held_blobs_.end(),
          std::find(held_blobs_.begin(), held_blobs_.end(), handle));

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
extensions/browser/blob_holder.cc:65: for
(std::vector<std::string>::const_iterator uuid_it = blob_uuids.begin();
this n*n loop is probably not so bad because these collections will never be
very large? another option could be to have the holder hold collection s and
identify each collection with an id

int BlobHolder::HoldBlobs(scoped_ptr<std::vector<...> > blobs_to_hold) {
  return idmapofownptrs.insert(blobs_to_hold.release());
}

BlobHolder::ReleaseBlobs(int held_collection_id) {
  idmapofownptrs.erase(held_collection_id);
}

and send just the collection_id in the ipc round trip

might be smaller more efficient code here (with a larger storage struct)

[tommycli, 2014-05-23 18:48:38]

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
File extensions/browser/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
extensions/browser/blob_holder.cc:42: DCHECK(!ContainsBlobHandle(blob.get()));
On 2014/05/23 17:31:36, michaeln wrote:
> might be able to simplify
> 
> DCHECK_EQ(held_blobs_.end(),
>           std::find(held_blobs_.begin(), held_blobs_.end(), handle));

Was possible. No longer possible since I switched to the multimap.

https://codereview.chromium.org/280393003/diff/260001/extensions/browser/blob...
extensions/browser/blob_holder.cc:65: for
(std::vector<std::string>::const_iterator uuid_it = blob_uuids.begin();
On 2014/05/23 17:31:36, michaeln wrote:
> this n*n loop is probably not so bad because these collections will never be
> very large? another option could be to have the holder hold collection s and
> identify each collection with an id
> 
> int BlobHolder::HoldBlobs(scoped_ptr<std::vector<...> > blobs_to_hold) {
>   return idmapofownptrs.insert(blobs_to_hold.release());
> }
> 
> BlobHolder::ReleaseBlobs(int held_collection_id) {
>   idmapofownptrs.erase(held_collection_id);
> }
> 
> and send just the collection_id in the ipc round trip
> 
> might be smaller more efficient code here (with a larger storage struct)

Yeah you're right this is a little hard to understand. In my case blob_uuids is
1 or 2 in size, but i agree that this looks kind of janky.

I changed it to a multimap<string, linked_ptr> to see if I can improve without
changing the interface.

The collection id thing would also work. I can do that if yoz also thinks that's
better.

[Yoyo Zhou, 2014-05-23 22:14:00]

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
File extensions/browser/blob_holder.cc (right):

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
extensions/browser/blob_holder.cc:77: continue;
nit: why not just use an if/else here

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
File extensions/browser/blob_holder.h (right):

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
extensions/browser/blob_holder.h:37: typedef std::multimap<std::string,
linked_ptr<content::BlobHandle> >
Why is this a multimap rather than a map? If uuids are the keys, shouldn't they
be unique?

[michaeln, 2014-05-23 23:49:41]

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
File extensions/browser/blob_holder.h (right):

https://chromiumcodereview.appspot.com/280393003/diff/300012/extensions/brows...
extensions/browser/blob_holder.h:37: typedef std::multimap<std::string,
linked_ptr<content::BlobHandle> >
On 2014/05/23 22:14:00, Yoyo Zhou wrote:
> Why is this a multimap rather than a map? If uuids are the keys, shouldn't
they
> be unique?

i can answer that, blobHandle != blob, there can be many blobHandles referring
to the same blob, the blob stays around until all handles to it are gone.

[tommycli, 2014-05-23 23:52:47]

yoz: addressed your comments

https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
File extensions/browser/blob_holder.cc (right):

https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
extensions/browser/blob_holder.cc:77: continue;
On 2014/05/23 22:14:00, Yoyo Zhou wrote:
> nit: why not just use an if/else here

Done.

https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
File extensions/browser/blob_holder.h (right):

https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
extensions/browser/blob_holder.h:37: typedef std::multimap<std::string,
linked_ptr<content::BlobHandle> >
On 2014/05/23 22:14:00, Yoyo Zhou wrote:
> Why is this a multimap rather than a map? If uuids are the keys, shouldn't
they
> be unique?

content::BlobHandle is copyable and roughly serves as a refcounting. There can
be multiple handles to the same Blob with a single UUID.

[Yoyo Zhou, 2014-05-24 00:03:39]

On 2014/05/23 23:52:47, tommycli wrote:
> yoz: addressed your comments
> 
>
https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
> File extensions/browser/blob_holder.cc (right):
> 
>
https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
> extensions/browser/blob_holder.cc:77: continue;
> On 2014/05/23 22:14:00, Yoyo Zhou wrote:
> > nit: why not just use an if/else here
> 
> Done.
> 
>
https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
> File extensions/browser/blob_holder.h (right):
> 
>
https://codereview.chromium.org/280393003/diff/300012/extensions/browser/blob...
> extensions/browser/blob_holder.h:37: typedef std::multimap<std::string,
> linked_ptr<content::BlobHandle> >
> On 2014/05/23 22:14:00, Yoyo Zhou wrote:
> > Why is this a multimap rather than a map? If uuids are the keys, shouldn't
> they
> > be unique?
> 
> content::BlobHandle is copyable and roughly serves as a refcounting. There can
> be multiple handles to the same Blob with a single UUID.

Oh ok. LGTM.

[tommycli, 2014-05-24 03:10:11]

tsepez: I should give you a chance to look at the revised extension_messages.h
again. If you're still happy, please check the CQ box. Thanks!

[tommycli, 2014-05-24 03:19:26]

michaeln, yoz: Thanks for all your help on this patch.

[Tom Sepez, 2014-05-27 18:15:33]

Messages LGTM.

[tommycli, 2014-06-02 16:13:25]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-06-02 16:13:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/280393003/340001

[commit-bot: I haz the power, 2014-06-02 17:28:13]

Change committed as 274268