Issue 1234813004: [BlobAsync] Asynchronous Blob Construction Final Patch

dmurph

dmurph@chromium.org changed reviewers: + michaeln@chromium.org

5 years, 4 months ago (2015-08-04 17:35:52 UTC) #1

dmurph

Hey there, This is the integration patch. It's not as big, yay! Things to still ...

5 years, 4 months ago (2015-08-04 17:35:53 UTC) #2

dmurph

dmurph@chromium.org changed reviewers: + kinuko@chromium.org

5 years, 4 months ago (2015-08-13 13:21:32 UTC) #3

dmurph

Hi Kinuko & Michaeln, Here is the final hookup patch for async blob transport. It's ...

5 years, 4 months ago (2015-08-13 13:22:08 UTC) #4

dmurph

Description was changed from ========== This is the final patch that hooks up the blob ...

5 years, 2 months ago (2015-10-19 23:46:38 UTC) #5

dmurph

Description was changed from ========== This is the final patch that hooks up the blob ...

5 years, 2 months ago (2015-10-19 23:46:54 UTC) #6

dmurph

Description was changed from ========== This is the final patch that hooks up the blob ...

5 years, 2 months ago (2015-10-20 23:55:19 UTC) #7

dmurph

This is it guys! The hookup patch for Async Blob transport! It's finally time to ...

5 years ago (2015-12-02 21:56:10 UTC) #9

dmurph

Description was changed from ========== [BlobAsync] Hookup patch. This is the final patch that hooks ...

5 years ago (2015-12-02 22:00:31 UTC) #10

dmurph

Description was changed from ========== [BlobAsync] Hookup patch. This is the final patch that hooks ...

5 years ago (2015-12-02 22:00:53 UTC) #11

kinuko

First round, not doing real review yet https://codereview.chromium.org/1234813004/diff/220001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/220001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode25 content/browser/blob_storage/blob_dispatcher_host.cc:25: BlobDispatcherHost::~BlobDispatcherHost() {} ...

5 years ago (2015-12-03 13:57:11 UTC) #12

dmurph

https://codereview.chromium.org/1234813004/diff/220001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/220001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode25 content/browser/blob_storage/blob_dispatcher_host.cc:25: BlobDispatcherHost::~BlobDispatcherHost() {} On 2015/12/03 at 13:57:10, kinuko wrote: > ...

5 years ago (2015-12-03 22:53:23 UTC) #13

kinuko

I'm a bit behind on review, let me still send some comments... I won't be ...

5 years ago (2015-12-08 15:40:15 UTC) #14

michaeln

Not a full review yet, but at least a first pass at some comments. I ...

5 years ago (2015-12-10 01:14:06 UTC) #15

dmurph

https://codereview.chromium.org/1234813004/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/240001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode56 content/browser/blob_storage/blob_dispatcher_host.cc:56: ignore_result(blob_storage_host_->RegisterBlobUUID(uuid)); On 2015/12/08 at 15:40:15, kinuko wrote: > nit: ...

5 years ago (2015-12-11 22:10:36 UTC) #16

kinuko

Maybe partly because this change is the one that is trying to combine everything this ...

5 years ago (2015-12-13 08:41:45 UTC) #17

michaeln

I'd vote to look for ways to simplify this. I wonder if flattening out the ...

5 years ago (2015-12-15 02:40:48 UTC) #18

michaeln

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileapi/blob_storage_host.cc File content/browser/fileapi/blob_storage_host.cc (right): https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileapi/blob_storage_host.cc#newcode54 content/browser/fileapi/blob_storage_host.cc:54: } What if BlobAsyncBuilderHost.ascync_blob_map_ is not empty? https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileapi/blob_storage_host.cc#newcode93 content/browser/fileapi/blob_storage_host.cc:93: ...

5 years ago (2015-12-15 20:55:58 UTC) #19

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:54: }
What if BlobAsyncBuilderHost.ascync_blob_map_ is not empty?

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:93: context_->AsWeakPtr(), uuid,
cancel));
On 2015/12/13 08:41:45, kinuko wrote:
> It's a bit confusing that we have many slightly different methods to cancel
> building blob with several indirect callbacks. I've tried to decrypt these...
> 
> cancel callback passed to this method is: DH::CancelBlob()
> DH::CancelBlob() does:
> - call Host::CancelBuildingBlob
> - Send(CancelBuildingBlobMsg)
> Host::CancelBuildingBlob does:
> - call Context::CleanupCanceledAsync
> - delete from map
> Context::CancelAsync does:
> - call Context::CleanupCanceledAsync
> - call given callback (which is DH::CancelBlob)
> 
> And in the line 81-84 case we do:
> - call given callback (which is DH::CancelBlob)
> - call Context::CleanupCanceledAsync
> - call AsyncBuilder::StopBuildingBlob to unregister from the builder
> 
> Are all these really organized?  For example CleanupCanceledAsync seems to be
> attempted multiple times in some call cases, and calling sequence is not
really
> fixed either?
> 
> Also I don't think adapter methods like
> Context::{Finish,Cancel}AsyncBlobTransfer should be placed on Context, which
> doesn't give clearer definition about what these methods are supposed to do
> without callback context.
> 
> Could we at least unify the canceling sequence used within this method?

Agreed that having these callbacks bound to Context methods is a tough read.

I was looking thru the asyncbuilder class and noticed that the callbacks are
always run while processing an incoming ipc msg. They're never run truely
asyncly with respect to the API directly called by the BlobStorageHost class.
The callbacks always occur from within either StartBuildingBlob() or
OnMemoryResponses() and I think at most one callback is invoked per
AsyncBuilderHost method call. Is that right and is that fair to say will remain
true always?

If so, here's an idea to help readability. Instead of using callbacks for those
return paths, return values...

result_code = builder->Start(id, type, item_infos, &memory_requests_out);
switch (result_code) {
 case DONE:
    // woohoo, we're done
    break;
 case NOTDONEYET:
    // send some memory requests
    break;
 case ERROR:
    // doh, something ain't right
    break;
 case BADMESSAGE:
    // yikes, something really ain't right
    break;
}

result_code = builder->AddMemoryResponse(id, memory_responses,
&memory_requests_out);
// similar maybe the same, maybe use a helper method for that part?

wdyt?

dmurph

You guys have great comments, just letting you know that I'm still working on this. ...

5 years ago (2015-12-17 01:52:56 UTC) #20

dmurph

Alrighty! There are 4 big changes: 1. I removed all callbacks for done and cancel, ...

5 years ago (2015-12-18 03:22:50 UTC) #21

Alrighty! There are 4 big changes:

1. I removed all callbacks for done and cancel, and instead I have a new
   enum that I return for these states, as well as the BAD_IPC state.
2. I added bad ipc handling, which involves having a status code.
3. I put the content type & disposition in the register method so we can
   grab references to blobs immediately after they are registered.
4. I moved transport strategy determination logic OUT of the
   BlobTransportStrategy class and into the BlobAsyncBuilderHost, which
   I think is way more readable.

I can have more thorough BAD_IPC codes for the bad_messages.h file. Let me
know if you want me to change any of this.


Thanks so much guys! Hopefully this is cleaner.

Daniel

https://codereview.chromium.org/1234813004/diff/260001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:127: void
BlobDispatcherHost::BlobDoneBuilding(const std::string& uuid) {
On 2015/12/13 at 08:41:44, kinuko wrote:
> nit: DoneBuildingBlob ?

Removed.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:26: // to bridge the output of the
BlobTransportStrategy and the message format.
On 2015/12/13 at 08:41:45, kinuko wrote:
> nit: double 'to'

Done.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:27: void NoopFileHandleAdapter(
On 2015/12/15 at 02:40:48, michaeln wrote:
> The name makes it sounds like a noop, which is definitely not true,
> RequestMemoryCallbackAdapter?

Done.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:51: iter != blobs_inuse_map_.end();
++iter) {
On 2015/12/13 at 08:41:44, kinuko wrote:
> nit: can use range for

Done.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:54: }
On 2015/12/15 at 20:55:57, michaeln wrote:
> What if BlobAsyncBuilderHost.ascync_blob_map_ is not empty?

Since we're storing a reference in our in other map, we call Decrement in the
context, and that will correctly destruct the blob.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:58: if (!context_.get() ||
uuid.empty() || context_->registry().GetEntry(uuid)) {
On 2015/12/15 at 02:40:48, michaeln wrote:
> context_->registry().GetEntry(uuid)
> 
> That looks like a fairly common test, I'd vote for retaining the
context->IsInUse() helper.

Done :)

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:78: return;
On 2015/12/13 at 08:41:44, kinuko wrote:
> For uuid.empty(), !GetEntry(uuid) and StartBuildingBlob returning false
cases-- should we return false from this method too and handle it as
BadMessageReceived?  (I remember we talked something like that when we worked on
async_builder's StartBuildingBlob, didn't we?)

Hooked up the bad message handling. I don't think we ever called the bad message
handler for blobs, so yay we're more annoying for malicious IPCers.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:93: context_->AsWeakPtr(), uuid,
cancel));
On 2015/12/15 at 20:55:57, michaeln wrote:
> On 2015/12/13 08:41:45, kinuko wrote:
> > It's a bit confusing that we have many slightly different methods to cancel
> > building blob with several indirect callbacks. I've tried to decrypt
these...
> > 
> > cancel callback passed to this method is: DH::CancelBlob()
> > DH::CancelBlob() does:
> > - call Host::CancelBuildingBlob
> > - Send(CancelBuildingBlobMsg)
> > Host::CancelBuildingBlob does:
> > - call Context::CleanupCanceledAsync
> > - delete from map
> > Context::CancelAsync does:
> > - call Context::CleanupCanceledAsync
> > - call given callback (which is DH::CancelBlob)
> > 
> > And in the line 81-84 case we do:
> > - call given callback (which is DH::CancelBlob)
> > - call Context::CleanupCanceledAsync
> > - call AsyncBuilder::StopBuildingBlob to unregister from the builder
> > 
> > Are all these really organized?  For example CleanupCanceledAsync seems to
be
> > attempted multiple times in some call cases, and calling sequence is not
really
> > fixed either?
> > 
> > Also I don't think adapter methods like
> > Context::{Finish,Cancel}AsyncBlobTransfer should be placed on Context, which
> > doesn't give clearer definition about what these methods are supposed to do
> > without callback context.
> > 
> > Could we at least unify the canceling sequence used within this method?
> 
> Agreed that having these callbacks bound to Context methods is a tough read.
> 
> I was looking thru the asyncbuilder class and noticed that the callbacks are
always run while processing an incoming ipc msg. They're never run truely
asyncly with respect to the API directly called by the BlobStorageHost class.
The callbacks always occur from within either StartBuildingBlob() or
OnMemoryResponses() and I think at most one callback is invoked per
AsyncBuilderHost method call. Is that right and is that fair to say will remain
true always?
> 
> If so, here's an idea to help readability. Instead of using callbacks for
those return paths, return values...
> 
> result_code = builder->Start(id, type, item_infos, &memory_requests_out);
> switch (result_code) {
>  case DONE:
>     // woohoo, we're done
>     break;
>  case NOTDONEYET:
>     // send some memory requests
>     break;
>  case ERROR:
>     // doh, something ain't right
>     break;
>  case BADMESSAGE:
>     // yikes, something really ain't right
>     break;
> }
> 
> result_code = builder->AddMemoryResponse(id, memory_responses,
&memory_requests_out);
> // similar maybe the same, maybe use a helper method for that part?
> 
> wdyt?

Did this!

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:101:
async_builder_.OnMemoryResponses(uuid, responses);
On 2015/12/13 at 08:41:45, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:106: return false;
On 2015/12/15 at 02:40:48, michaeln wrote:
> Should this involve decrementing the refcount now that references to blobs can
be passed around prior to them being fully built?

Sure, we can do it that way.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.h (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.h:5: #ifndef
CONTENT_BROWSER_FILEAPI_BLOB_STORAGE_HOST_H_
On 2015/12/15 at 02:40:48, michaeln wrote:
> probably should move this file and its .cc to the browser/blob_storage dir

I want to avoid file moving until after all of these changes, it complicates
things a lot w.r.t reviewing (it looks like a new file for a couple of these).

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/fileapi_message_filter.cc:55: const uint32
kFilteredMessageClasses[] = {BlobMsgStart, FileSystemMsgStart};
On 2015/12/15 at 02:40:48, michaeln wrote:
> does BlobMsgStart still belong here given the new filter class?

I don't think so.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.h (right):

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.h:44: // RunWhenConstructionIsComplete to
wait.
On 2015/12/13 at 08:41:45, kinuko wrote:
> RunOnConstructionComplete?

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.h:46: bool IsAsyncConstructing() const;
On 2015/12/13 at 08:41:45, kinuko wrote:
> IsAsyncConstructing --> should this be called IsBeingBuilt?  (As was commented
before) using different names is confusing.

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.h:50: // immediately on the current
message loop.
On 2015/12/13 at 08:41:45, kinuko wrote:
> nit: "...on the current message (i.e. IO thread)"

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:39: if (!entry || entry->state !=
BlobState::ACTIVE || entry->exceeded_memory) {
On 2015/12/15 at 02:40:48, michaeln wrote:
> If blobs can be passed around while they're being built, do we need this
method to allow them to be looked up prior to being done?

Fixed. I put the type and disposition as part of the initial IPC that we use to
register the blob.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:40: LOG(ERROR) << "Could not find
blob " << uuid;
On 2015/12/15 at 02:40:48, michaeln wrote:
> I don't think this warrants logging.

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:55: LOG(ERROR) << "Could not find
blob url " << url;
On 2015/12/15 at 02:40:48, michaeln wrote:
> Ditto doesn't need to be logged here and below.

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:132: for (const auto& callback :
entry->construction_complete_callbacks) {
On 2015/12/13 at 08:41:45, kinuko wrote:
> We've done just 'building' the blob per the method name, could we call this
build_completion_callback ?

Done.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:177: const
base::Callback<void(bool)>& done) {
On 2015/12/13 at 08:41:45, kinuko wrote:
> If the caller's always check Context::IsBeingBuilt (or
DataHandle::IsAsyncConstructing()) before calling RunOnConstructionComplete I'd
just DCHECK registry_.GetEntry(uuid) is valid and entry->state is not ACTIVE,
and clearly document that this shouldn't be called when IsBeingBuilt returns
false.

Done, and also done for below.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:214: case BlobState::CONSTRUCTION:
On 2015/12/15 at 02:40:48, michaeln wrote:
> How is this case reachable? Looks like the state transition from
ASYNC_TRANSPORTATION to ACTIVE happens in a function call in the io thread. So
how can this method be invoked while it's in the intermediary CONSTRUCTION
state.
> 
> I'm wondering if we need to the distinction between BlobState CONSTRUCTION and
ASYNC_TRANSPORTATION. We need to know "it's not done yet", but why do we need to
know in more detail than that.

It doesn't look reachable. I'll check fail.

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:92: //
##################################
On 2015/12/13 at 08:41:45, kinuko wrote:
> nit: as was discussed we generally prefer not using these marks in comments

Done.

palmer

https://codereview.chromium.org/1234813004/diff/320001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/320001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode33 content/browser/blob_storage/blob_dispatcher_host.cc:33: void BlobDispatcherHost::OnChannelConnected(int32 peer_pid) { |peer_pid| is not used? Is ...

4 years, 11 months ago (2016-01-05 23:49:21 UTC) #22

kinuko

I think it's now more readable after a lot of refactoring. It's still getting a ...

4 years, 11 months ago (2016-01-07 07:46:50 UTC) #23

dmurph

Thanks for the reviews! Kinuko: there's a question in there about your opinion on callbacks ...

4 years, 11 months ago (2016-01-07 22:57:13 UTC) #24

kinuko

https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode76 content/browser/blob_storage/blob_dispatcher_host.cc:76: base::Bind(&BlobDispatcherHost::SendMemoryRequest, this, uuid)); On 2016/01/07 22:57:13, dmurph wrote: > ...

4 years, 11 months ago (2016-01-08 09:20:03 UTC) #25

https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:76:
base::Bind(&BlobDispatcherHost::SendMemoryRequest, this, uuid));
On 2016/01/07 22:57:13, dmurph wrote:
> On 2016/01/07 at 07:46:50, kinuko wrote:
> > Now that we have BlobDispatcherHost can we stop using callback for this too?

> It sounds like we can simply pass IPC::Sender pointer or something so that the
> caller can just do sender->Send(new BlobStorageMsg_RequestMemoryItem).  Since
> blob_storage_host_ is owned by this instance we can probably guarantee that
the
> sender is alive when host's trying to call it?  Mocking IPC::Sender wouldn't
be
> that hard either, and it'll allow us to remove several indirections & adapter
> function.  Wdyt?
> 
> Unfortunately we can't use IPC::Sender in the storage/ directory. I could make
a
> pure-virtual interface, would that be better?

Putting aside whether it's preferred or not, I don't think we can't use
IPC::Sender in the storage/ directory (it's allowed in most directories).
However doing so would require moving some message definitions from content/ to
storage/... that'd be a bit tough.

> In the future for disk-based, we need to store the following callbacks to
allow
> the browser to wait while files are created and data is paged to disk:
> * Requesting memory when we're able to fit the memory (if we were waiting on
> paging stuff to disk)
> * Canceling the transfer if there was a file error.
> 
> I'm not sure what the cleanest way to do this is, so I just put it in
callbacks.
>  I could also typedef the callback if that makes it read better. Let me know
> what you think.

I can't tell what architecture would be better without actually seeing what
we'll have in the future, but if the blob_storage_host is expected to work with
some mechanism the role would become clearer if we introduce interface. (While
that's also true that it could make the code complex too)

If we were to introduce a pure virtual interface for these callbacks what it'd
look like and how you'd name it?  Could you write a short pseudo code here so
that I can have a better idea?

kinuko

Some more comments https://codereview.chromium.org/1234813004/diff/380001/content/browser/blob_storage/blob_async_transport_strategy_unittest.cc File content/browser/blob_storage/blob_async_transport_strategy_unittest.cc (right): https://codereview.chromium.org/1234813004/diff/380001/content/browser/blob_storage/blob_async_transport_strategy_unittest.cc#newcode74 content/browser/blob_storage/blob_async_transport_strategy_unittest.cc:74: // and we save to one ...

4 years, 11 months ago (2016-01-08 14:52:08 UTC) #26

dmurph

On 2016/01/08 at 09:20:03, kinuko wrote: > https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_storage/blob_dispatcher_host.cc > File content/browser/blob_storage/blob_dispatcher_host.cc (right): > > https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode76 ...

4 years, 11 months ago (2016-01-12 00:53:05 UTC) #27

On 2016/01/08 at 09:20:03, kinuko wrote:
>
https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
> File content/browser/blob_storage/blob_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
> content/browser/blob_storage/blob_dispatcher_host.cc:76:
base::Bind(&BlobDispatcherHost::SendMemoryRequest, this, uuid));
> On 2016/01/07 22:57:13, dmurph wrote:
> > On 2016/01/07 at 07:46:50, kinuko wrote:
> > > Now that we have BlobDispatcherHost can we stop using callback for this
too? 
> > It sounds like we can simply pass IPC::Sender pointer or something so that
the
> > caller can just do sender->Send(new BlobStorageMsg_RequestMemoryItem). 
Since
> > blob_storage_host_ is owned by this instance we can probably guarantee that
the
> > sender is alive when host's trying to call it?  Mocking IPC::Sender wouldn't
be
> > that hard either, and it'll allow us to remove several indirections &
adapter
> > function.  Wdyt?
> > 
> > Unfortunately we can't use IPC::Sender in the storage/ directory. I could
make a
> > pure-virtual interface, would that be better?
> 
> Putting aside whether it's preferred or not, I don't think we can't use
IPC::Sender in the storage/ directory (it's allowed in most directories).
However doing so would require moving some message definitions from content/ to
storage/... that'd be a bit tough.
> 
> > In the future for disk-based, we need to store the following callbacks to
allow
> > the browser to wait while files are created and data is paged to disk:
> > * Requesting memory when we're able to fit the memory (if we were waiting on
> > paging stuff to disk)
> > * Canceling the transfer if there was a file error.
> > 
> > I'm not sure what the cleanest way to do this is, so I just put it in
callbacks.
> >  I could also typedef the callback if that makes it read better. Let me know
> > what you think.
> 
> I can't tell what architecture would be better without actually seeing what
we'll have in the future, but if the blob_storage_host is expected to work with
some mechanism the role would become clearer if we introduce interface. (While
that's also true that it could make the code complex too)
> 
> If we were to introduce a pure virtual interface for these callbacks what it'd
look like and how you'd name it?  Could you write a short pseudo code here so
that I can have a better idea?

The complexity comes from the 'cancel' pipeline.  Canceling will asynchronously
happen when we have failures creating files. This will cancel both the blob that
we're creating files for, but also the blobs waiting to be saved (either
straight to file or they're waiting for blobs to be paged to disk).

The cancel pipeline (for both async and sync) currently clears state as it goes
back to the sender. For example, when the file operation fails, then the
following states need to be cleared:
* Blob Async Builder
* Blob Storage Context
* Blob Storage Host

In a callback-style setup, the async builder will handle the first callback,
which clears it's internal state and calls clear on the BlobStorageContext. 
Then it calls the callback it was given by the DispatcherHost, which will then
send the cancel message. If we did they with an interface, we would have to
either:
1. Have the high-level implementer (BlobDispatcherHost) call back into the
BlobStorageHost to clear all state for that blob.
2. Have the blob async builder re-implement the interface and delegate, so it
can handle clearing it's state.

I not a huge fan of #1, because when we decide to cancel blob transfer anywhere,
we are leaving our objects in a strange state where we expect our state to be
cleared separately. #2 seems like a little overkill, but it would work just
fine.

RequestMemory doesn't need to modify anything along the pipeline, so it can just
be an interface, where the implementation stores a refcount of the
BrowserMessageFilter (BlobDispatcherHost).

so.... does that make sense? What do you think?

dmurph

On 2016/01/12 at 00:53:05, dmurph wrote: > On 2016/01/08 at 09:20:03, kinuko wrote: > > ...

4 years, 11 months ago (2016-01-12 01:09:24 UTC) #28

On 2016/01/12 at 00:53:05, dmurph wrote:
> On 2016/01/08 at 09:20:03, kinuko wrote:
> >
https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
> > File content/browser/blob_storage/blob_dispatcher_host.cc (right):
> > 
> >
https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
> > content/browser/blob_storage/blob_dispatcher_host.cc:76:
base::Bind(&BlobDispatcherHost::SendMemoryRequest, this, uuid));
> > On 2016/01/07 22:57:13, dmurph wrote:
> > > On 2016/01/07 at 07:46:50, kinuko wrote:
> > > > Now that we have BlobDispatcherHost can we stop using callback for this
too? 
> > > It sounds like we can simply pass IPC::Sender pointer or something so that
the
> > > caller can just do sender->Send(new BlobStorageMsg_RequestMemoryItem). 
Since
> > > blob_storage_host_ is owned by this instance we can probably guarantee
that the
> > > sender is alive when host's trying to call it?  Mocking IPC::Sender
wouldn't be
> > > that hard either, and it'll allow us to remove several indirections &
adapter
> > > function.  Wdyt?
> > > 
> > > Unfortunately we can't use IPC::Sender in the storage/ directory. I could
make a
> > > pure-virtual interface, would that be better?
> > 
> > Putting aside whether it's preferred or not, I don't think we can't use
IPC::Sender in the storage/ directory (it's allowed in most directories).
However doing so would require moving some message definitions from content/ to
storage/... that'd be a bit tough.
> > 
> > > In the future for disk-based, we need to store the following callbacks to
allow
> > > the browser to wait while files are created and data is paged to disk:
> > > * Requesting memory when we're able to fit the memory (if we were waiting
on
> > > paging stuff to disk)
> > > * Canceling the transfer if there was a file error.
> > > 
> > > I'm not sure what the cleanest way to do this is, so I just put it in
callbacks.
> > >  I could also typedef the callback if that makes it read better. Let me
know
> > > what you think.
> > 
> > I can't tell what architecture would be better without actually seeing what
we'll have in the future, but if the blob_storage_host is expected to work with
some mechanism the role would become clearer if we introduce interface. (While
that's also true that it could make the code complex too)
> > 
> > If we were to introduce a pure virtual interface for these callbacks what
it'd look like and how you'd name it?  Could you write a short pseudo code here
so that I can have a better idea?
> 
> The complexity comes from the 'cancel' pipeline.  Canceling will
asynchronously happen when we have failures creating files. This will cancel
both the blob that we're creating files for, but also the blobs waiting to be
saved (either straight to file or they're waiting for blobs to be paged to
disk).
> 
> The cancel pipeline (for both async and sync) currently clears state as it
goes back to the sender. For example, when the file operation fails, then the
following states need to be cleared:
> * Blob Async Builder
> * Blob Storage Context
> * Blob Storage Host
> 
> In a callback-style setup, the async builder will handle the first callback,
which clears it's internal state and calls clear on the BlobStorageContext. 
Then it calls the callback it was given by the DispatcherHost, which will then
send the cancel message. If we did they with an interface, we would have to
either:
> 1. Have the high-level implementer (BlobDispatcherHost) call back into the
BlobStorageHost to clear all state for that blob.
> 2. Have the blob async builder re-implement the interface and delegate, so it
can handle clearing it's state.
> 
> I not a huge fan of #1, because when we decide to cancel blob transfer
anywhere, we are leaving our objects in a strange state where we expect our
state to be cleared separately. #2 seems like a little overkill, but it would
work just fine.
> 
> RequestMemory doesn't need to modify anything along the pipeline, so it can
just be an interface, where the implementation stores a refcount of the
BrowserMessageFilter (BlobDispatcherHost).
> 
> so.... does that make sense? What do you think?

I forgot to mention, for RequestMemory:
At the BlobDispatcherHost level, we are sending an IPC::PlatformFileForTransit
for the file argument. This is only available in content/ (not storage/), so we
have to use something different in storage/ (probably base::File). This
transformation is currently happening in BlobStorageHost in an intermediate
callback. If we turned this into an interface, we could just do the
transformation in the impl of our interface, in BlobDispatcherHost's cc file.

michaeln

From a functional point of view, I think subverting refcounting semantics is probably not a ...

4 years, 11 months ago (2016-01-13 02:42:44 UTC) #29

From a functional point of view, I think subverting refcounting semantics is
probably not a good idea.

From a structural point of view, i think there are still too many moving parts
and they're getting in the way.

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/1234813004/diff/260001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:58: if (!context_.get() ||
uuid.empty() || context_->registry().GetEntry(uuid)) {
On 2015/12/18 03:22:49, dmurph wrote:
> On 2015/12/15 at 02:40:48, michaeln wrote:
> > context_->registry().GetEntry(uuid)
> > 
> > That looks like a fairly common test, I'd vote for retaining the
> context->IsInUse() helper.
> 
> Done :)

what happened to this change?

https://codereview.chromium.org/1234813004/diff/280001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/280001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:150:
blob_storage_host_->DecrementBlobRefCount(uuid);
It'd be nice to detect the refcounting bugs instead of letting them slide
silently by. Ideally, this would be considered a badipc. If we respected
refcounting semantics, could we do that?

https://codereview.chromium.org/1234813004/diff/280001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/1234813004/diff/280001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:79: if
(!context_->MaybeStartBlobTransfer(uuid)) {
The name of this method is very misleading. It sounds like it initiates an asyn
async process but really it does almost nothing. It sets a bit on the blob
entry. But does that bit need to be carried by the central registry? I don't
think it does, i think forcing this state to be present there is hurting more
than helping.

Here's an earlier comment on this topic...
=====
https://codereview.chromium.org/1234813004/diff/260001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:214: case BlobState::CONSTRUCTION:
How is this case reachable? Looks like the state transition from
ASYNC_TRANSPORTATION to ACTIVE happens in a function call in the io thread. So
how can this method be invoked while it's in the intermediary CONSTRUCTION
state.

I'm wondering if we need to the distinction between BlobState CONSTRUCTION and
ASYNC_TRANSPORTATION. We need to know "it's not done yet", but why do we need to
know in more detail than that.
=====

More concretely, I think we can reduce the number of BlobStates and in doing so
simplify/clarify some code. When a uuid is initially registered, its pending, if
everything goes well it becomes complete, if something goes wrong its broken.

PENDING
COMPLETE
BROKEN

There can be many references to a blob in any of these states. A state
transition to BROKEN does not break those references. References are managed via
a refcounting mechanism.

https://codereview.chromium.org/1234813004/diff/280001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:143:
context_->DecrementBlobRefCount(uuid_refcount->first);
Smashing the refcount to zero doesn't look right to me? Since blob references
can be passed around prior to completion, other consumers besides the 'builder'
of the blob can hold references. I think those references should remain valid
until those consumers drop their references.

https://codereview.chromium.org/1234813004/diff/280001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:158: return false;
If a 'false' return value is interpreted as a "badipc", smashing refcounts and
deleting the central map entry in cancel would result in crashing renderers.

https://codereview.chromium.org/1234813004/diff/320001/content/browser/fileap...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/1234813004/diff/320001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:61: if (!context_.get() ||
uuid.empty() || context_->registry().HasEntry(uuid)) {
IsInUse()?

https://codereview.chromium.org/1234813004/diff/320001/content/browser/fileap...
content/browser/fileapi/blob_storage_host.cc:149: if (!context_.get() ||
!context_->registry().HasEntry(uuid))
IsInUse() helper

https://codereview.chromium.org/1234813004/diff/320001/content/child/webblobr...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/1234813004/diff/320001/content/child/webblobr...
content/child/webblobregistry_impl.cc:243: uuid, consolidation.Pass(),
sender.get());
can this use std::move?

https://codereview.chromium.org/1234813004/diff/320001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1234813004/diff/320001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.cc:104: return
context_->CreateSnapshot(uuid_).Pass();
is .Pass needed?

https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/340001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:76:
base::Bind(&BlobDispatcherHost::SendMemoryRequest, this, uuid));
On 2016/01/08 09:20:03, kinuko wrote:
> On 2016/01/07 22:57:13, dmurph wrote:
> > On 2016/01/07 at 07:46:50, kinuko wrote:
> > > Now that we have BlobDispatcherHost can we stop using callback for this
too?
> 
> > It sounds like we can simply pass IPC::Sender pointer or something so that
the
> > caller can just do sender->Send(new BlobStorageMsg_RequestMemoryItem). 
Since
> > blob_storage_host_ is owned by this instance we can probably guarantee that
> the
> > sender is alive when host's trying to call it?  Mocking IPC::Sender wouldn't
> be
> > that hard either, and it'll allow us to remove several indirections &
adapter
> > function.  Wdyt?
> > 
> > Unfortunately we can't use IPC::Sender in the storage/ directory. I could
make
> a
> > pure-virtual interface, would that be better?
> 
> Putting aside whether it's preferred or not, I don't think we can't use
> IPC::Sender in the storage/ directory (it's allowed in most directories).
> However doing so would require moving some message definitions from content/
to
> storage/... that'd be a bit tough.
> 
> > In the future for disk-based, we need to store the following callbacks to
> allow
> > the browser to wait while files are created and data is paged to disk:
> > * Requesting memory when we're able to fit the memory (if we were waiting on
> > paging stuff to disk)
> > * Canceling the transfer if there was a file error.
> > 
> > I'm not sure what the cleanest way to do this is, so I just put it in
> callbacks.
> >  I could also typedef the callback if that makes it read better. Let me know
> > what you think.
> 
> I can't tell what architecture would be better without actually seeing what
> we'll have in the future, but if the blob_storage_host is expected to work
with
> some mechanism the role would become clearer if we introduce interface. (While
> that's also true that it could make the code complex too)
> 
> If we were to introduce a pure virtual interface for these callbacks what it'd
> look like and how you'd name it?  Could you write a short pseudo code here so
> that I can have a better idea?

==== dmurph said...
The complexity comes from the 'cancel' pipeline.  Canceling will
asynchronously happen when we have failures creating files. This will cancel
both the blob that we're creating files for, but also the blobs waiting to be
saved (either straight to file or they're waiting for blobs to be paged to
disk).

The cancel pipeline (for both async and sync) currently clears state as it
goes back to the sender. For example, when the file operation fails, then the
following states need to be cleared:
 * Blob Async Builder
 * Blob Storage Context
 * Blob Storage Host
====

I think some of that complexity is a "manmade" problem so to speak. Why is the
state so diffuse such that the cancel logic has to touch so many different
things? The shape of a solution is to change how the state is represented to
make it easier to perform the functions that we need to perform (like canceling
and sending messages requesting the data from the renderer).

I'm not sure the divisions between BlobStorageHost, BlobDispatcherHost, and
BlobAsyncBuilderHost are helping more than their hurting. Seems like the first
two are very naturally  consolidated into one class, and honestly, I think the
third could join that party as well. Question: if BlobStorageHost and
BlobDispatcherHost and BlobAsyncBuilderHost are all one and the same class...
does that make it easier or harder to send the memory requests?

Also, extra complexity will be needed when async fileio happens under the
covers, but that's not needed yet since there is not yet any under the conver
fileio.

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:57: if (!entry) {
should testing for the exceeded_memory condition happen here too?

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:69:
BuildAndStoreBlob(external_builder);
I think an explicit call to RegisterBlobUUID() prior to calling
BuildAndStoreBlob() would eliminate the squishy conditional logic  in the latter
method.

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:94: LOG(ERROR) << "Could not revoke
url";
Please avoid unwarranted LOG'ing.

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:104: entry =
registry_.CreateEntry(external_builder.uuid());
I'm wary of squishy conditional logic like this because its hard to distinguish
a programming error or bug from correct expected behavior.

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:120: // our content type and
disposition from the RegisterBlobUUID call.
ditto squishy conditional logic

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:151: LOG(ERROR) << "Snapshot
requested for incomplete blob";
This used to be a DCHECK'd condition, now there's a return path for it, is
requesting snapshots of incomplete blobs now expected behavior? Under what
circumstance would that be expected? Since an error is being logged probably its
not expected?

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:448: bool
BlobStorageContext::MaybeStartBlobTransfer(const std::string& uuid) {
Given the simpler set of states, I don't think this method needs to be here at
all. The uuid is registered in the pending state and it's still pending at this
point in time.

https://codereview.chromium.org/1234813004/diff/380001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:473: registry_.DeleteEntry(uuid);
Deleting the uuid's entry doesn't look right given refcounting is being used to
manage individual references. Doing this makes it a lot harder to detect
refcounting bugs since you can't tell between 'it was canceled' and 'we messed
up the refcount' when an entry for an id is not found.

dmurph

Initial reply for some of the cancel architecting: I'm currently merging BlobStorageHost and BlobDispatcherHost into ...

4 years, 11 months ago (2016-01-13 02:50:11 UTC) #30

dmurph

On 2016/01/13 at 02:50:11, dmurph wrote: > Initial reply for some of the cancel architecting: ...

4 years, 11 months ago (2016-01-14 20:40:03 UTC) #31

dmurph

Alright, I incorporated the suggestions and I hope this looks cleaner and more understandable ;) ...

4 years, 11 months ago (2016-01-16 00:43:40 UTC) #32

kinuko

Thanks for drastically reworking on this. The patch's getting bigger which is a bit tough ...

4 years, 11 months ago (2016-01-19 14:31:40 UTC) #33

dmurph

On 2016/01/19 at 14:31:40, kinuko wrote: > Thanks for drastically reworking on this. The patch's ...

4 years, 11 months ago (2016-01-19 20:28:30 UTC) #34

kinuko

Haven't looked at most tests yet. This version starts to feel readable to me :) ...

4 years, 11 months ago (2016-01-20 14:14:21 UTC) #36

dmurph

Done. IPC::TestSink is pretty great, thanks. https://codereview.chromium.org/1234813004/diff/460001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/460001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode61 content/browser/blob_storage/blob_dispatcher_host.cc:61: sender_ = nullptr; ...

4 years, 11 months ago (2016-01-20 22:57:06 UTC) #37

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 11 months ago (2016-01-22 00:14:16 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/540001

4 years, 11 months ago (2016-01-22 00:16:07 UTC) #39

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-22 01:09:28 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/162818)

4 years, 11 months ago (2016-01-22 01:09:29 UTC) #41

michaeln

i haven't looked at the tests yet either but i think this is shaping up ...

4 years, 11 months ago (2016-01-25 20:24:09 UTC) #42

i haven't looked at the tests yet either but i think this is shaping up

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:36: void
BlobDispatcherHost::OnFilterAdded(IPC::Sender* sender) {
I think this breaks the BMF::Send() method? The base class is expecting it's
OnFilterAdded method to be called
see comments in the unittest file about not needing this override or
ShutdownForBadMessage

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:44: sender_ = nullptr;
since |this| is a sender, is this data member needed? you can override the
Send() method to hook for testing

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:63: void
BlobDispatcherHost::ShutdownForBadMessage() {
nuke me and hook for testing elsewhere

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:69: 
can remove some blank lines here

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:113:
context->FlagBlobAsBrokenAndBuild(uuid, code);
nit: context()->DoSomething() is a little more concise

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:290:
context->MaybeBreakAndBuildBlob(uuid_refnum_pair.first);
Wait, this doesn't look correct? Why maybe break all blobs this renderer happens
to refer to? Some other process may be responsible for building it?

see other comments about PendingBlobDataHandle class to help clarify ownership
of the pending'ness attribute

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:53: }  //
namespace
I think you could put a TestableBlobDispatcherHost here that overrides two
methods.

TestableBlobDispatcherHost : public BlobDispatcherHost {
  void Send(IPC::Message* msg) override;
  void ShutdownForBadmessage() override;

  bool yikes_was_shutdown_for_bad_message_;
  IPC::TestSink sink_;
};

And avoid the production code overrides and unused data members.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:92: return
BlobTransportResult::DONE;
seems like this class could directly context->CompletePendingBlob() or
contxt->CancelPendingBlob(code) when processing these messages, then
BDHost::MaybeFinishBlobTranfer and SendIPCResonse might not be needed and the
control flow could go more in a straight line... wdyt?

BDHost::OnBuilderishMethod() {
  rv = builder_->PleasePerformAllAspectsOfBuilderishMethod();
  if (rv == DONE)
    Send(new BlobStorageMsg_DoneBuildingBlob(uuid));
  else if (rv == bad_ipc)
    ReceivedBadMessage();
  else
    Send(new BlobStorageMsg_CancelBuildingBlob(uuid, rv));
}

Do we really need two different set of result codes for this?
   BlobTransportResult vs IPCBlobCreationCancelCode
If we really do need to enums, can one values be defined in terms of the other
so we can cast instead of having switch/if statements?

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:226:
BlobAsyncTransportRequestBuilder& builder = state->request_builder;
maybe swap the order of lines 224 & 226 so the 'builder' local can be used on
the latter

const std::vector<MemoryItemRequest>& requests = builder.requests()

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:97: void BuildRegisteredBlob(const
BlobDataBuilder& external_builder);
Maybe call this CompletePendingBlob() to harmonize with the state names?

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:108: void RegisterBlobUUID(const
std::string& uuid,
I think this method could return a PendingBlobDataHandle whose dtor marks it as
broken if it hasn't already been completed or deleted. The asyncblobbuilder host
would ownptr a pendinghandle for all blobs it's responsible for building. This
handle probably shouldn't add a ref since we want blobs in the midst of being
built to go poof if all references are dropped prior to completion.

That might could simplify the cancel() code paths. Wdyt?

If the PendingBlobDataHandle is deleted prior to CompletePendingBlob() having
been called, you'd  get a blob in the BROKEN state. As soon as something that
warrants a transition to the broken state... tell the asynBuilder to stop(uuid),
it deletes the pending handle, and its done.


PendingBlobDataHandle::~PendingBlobDataHandle() {
  if (!context_) return; // weakptr to the context
  entry = context_->registry_.GetEntry(uuid_);
  if (!entry || entry->state != PENDING) return;
  entry->state = BROKEN;
}

with that in mind, maybe call this method CreatePendingBlob(uuid)

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:121: void
MaybeBreakAndBuildBlob(const std::string& uuid);
There shouldn't be any doubt about which blobs to cancel building the 'host'
knows exactly which are being built in it. The asycnbhuilder host has a map with
the uuids.

I was wondering what happened to the CancelBuildingBlob() method? It's been
replaced with two methods that perform that function but with cryptic names
FlagBlobAsBrokenAndBuild() MaybeBreakAndBuildBlob().

see comments about the PendingBlobDataHandle class

dmurph

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode36 content/browser/blob_storage/blob_dispatcher_host.cc:36: void BlobDispatcherHost::OnFilterAdded(IPC::Sender* sender) { On 2016/01/25 at 20:24:08, michaeln ...

4 years, 10 months ago (2016-02-03 20:08:50 UTC) #43

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:36: void
BlobDispatcherHost::OnFilterAdded(IPC::Sender* sender) {
On 2016/01/25 at 20:24:08, michaeln wrote:
> I think this breaks the BMF::Send() method? The base class is expecting it's
OnFilterAdded method to be called
> see comments in the unittest file about not needing this override or
ShutdownForBadMessage

ok, I'll do it that way.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:44: sender_ = nullptr;
On 2016/01/25 at 20:24:08, michaeln wrote:
> since |this| is a sender, is this data member needed? you can override the
Send() method to hook for testing

I don't like inheritance for testing, but I guess it works in this case.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:63: void
BlobDispatcherHost::ShutdownForBadMessage() {
On 2016/01/25 at 20:24:09, michaeln wrote:
> nuke me and hook for testing elsewhere

Done.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:69: 
On 2016/01/25 at 20:24:08, michaeln wrote:
> can remove some blank lines here

Done.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:113:
context->FlagBlobAsBrokenAndBuild(uuid, code);
On 2016/01/25 at 20:24:08, michaeln wrote:
> nit: context()->DoSomething() is a little more concise

Done.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:290:
context->MaybeBreakAndBuildBlob(uuid_refnum_pair.first);
On 2016/01/25 at 20:24:08, michaeln wrote:
> Wait, this doesn't look correct? Why maybe break all blobs this renderer
happens to refer to? Some other process may be responsible for building it?
> 
> see other comments about PendingBlobDataHandle class to help clarify ownership
of the pending'ness attribute

Done. Moved context handling into the async builder.

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:53: }  //
namespace
On 2016/01/25 at 20:24:09, michaeln wrote:
> I think you could put a TestableBlobDispatcherHost here that overrides two
methods.
> 
> TestableBlobDispatcherHost : public BlobDispatcherHost {
>   void Send(IPC::Message* msg) override;
>   void ShutdownForBadmessage() override;
> 
>   bool yikes_was_shutdown_for_bad_message_;
>   IPC::TestSink sink_;
> };
> 
> And avoid the production code overrides and unused data members.

Done.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:92: return
BlobTransportResult::DONE;
On 2016/01/25 at 20:24:09, michaeln wrote:
> seems like this class could directly context->CompletePendingBlob() or
contxt->CancelPendingBlob(code) when processing these messages, then
BDHost::MaybeFinishBlobTranfer and SendIPCResonse might not be needed and the
control flow could go more in a straight line... wdyt?
> 
> BDHost::OnBuilderishMethod() {
>   rv = builder_->PleasePerformAllAspectsOfBuilderishMethod();
>   if (rv == DONE)
>     Send(new BlobStorageMsg_DoneBuildingBlob(uuid));
>   else if (rv == bad_ipc)
>     ReceivedBadMessage();
>   else
>     Send(new BlobStorageMsg_CancelBuildingBlob(uuid, rv));
> }
> 
> Do we really need two different set of result codes for this?
>    BlobTransportResult vs IPCBlobCreationCancelCode
> If we really do need to enums, can one values be defined in terms of the other
so we can cast instead of having switch/if statements?

We now call directly into the context.

One of those enums is for the browser blob system to specify if we're a bad ipc,
done with transport, pending memory responses, or we need to cancel. The second
is for enumerating cancel conditions.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:226:
BlobAsyncTransportRequestBuilder& builder = state->request_builder;
On 2016/01/25 at 20:24:09, michaeln wrote:
> maybe swap the order of lines 224 & 226 so the 'builder' local can be used on
the latter
> 
> const std::vector<MemoryItemRequest>& requests = builder.requests()

Done.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:97: void BuildRegisteredBlob(const
BlobDataBuilder& external_builder);
On 2016/01/25 at 20:24:09, michaeln wrote:
> Maybe call this CompletePendingBlob() to harmonize with the state names?

Done.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:108: void RegisterBlobUUID(const
std::string& uuid,
On 2016/01/25 at 20:24:09, michaeln wrote:
> I think this method could return a PendingBlobDataHandle whose dtor marks it
as broken if it hasn't already been completed or deleted. The asyncblobbuilder
host would ownptr a pendinghandle for all blobs it's responsible for building.
This handle probably shouldn't add a ref since we want blobs in the midst of
being built to go poof if all references are dropped prior to completion.
> 
> That might could simplify the cancel() code paths. Wdyt?
> 
> If the PendingBlobDataHandle is deleted prior to CompletePendingBlob() having
been called, you'd  get a blob in the BROKEN state. As soon as something that
warrants a transition to the broken state... tell the asynBuilder to stop(uuid),
it deletes the pending handle, and its done.
> 
> 
> PendingBlobDataHandle::~PendingBlobDataHandle() {
>   if (!context_) return; // weakptr to the context
>   entry = context_->registry_.GetEntry(uuid_);
>   if (!entry || entry->state != PENDING) return;
>   entry->state = BROKEN;
> }
> 
> with that in mind, maybe call this method CreatePendingBlob(uuid)

With blob async builder host calling directly into the context, we no longer
need this.  Thanks for the suggestion though.

https://codereview.chromium.org/1234813004/diff/540001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:121: void
MaybeBreakAndBuildBlob(const std::string& uuid);
On 2016/01/25 at 20:24:09, michaeln wrote:
> There shouldn't be any doubt about which blobs to cancel building the 'host'
knows exactly which are being built in it. The asycnbhuilder host has a map with
the uuids.
> 
> I was wondering what happened to the CancelBuildingBlob() method? It's been
replaced with two methods that perform that function but with cryptic names
FlagBlobAsBrokenAndBuild() MaybeBreakAndBuildBlob().
> 
> see comments about the PendingBlobDataHandle class

Removed this per all context calls being handled by the blob async builder host.

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 10 months ago (2016-02-03 20:09:38 UTC) #44

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/580001

4 years, 10 months ago (2016-02-03 20:11:44 UTC) #45

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 10 months ago (2016-02-03 21:08:15 UTC) #46

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/168760)

4 years, 10 months ago (2016-02-03 21:08:17 UTC) #47

michaeln

still haven't made it to the tests, i found a couple of simple bugs and ...

4 years, 10 months ago (2016-02-06 00:08:40 UTC) #48

dmurph

https://codereview.chromium.org/1234813004/diff/580001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/580001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode223 content/browser/blob_storage/blob_dispatcher_host.cc:223: return IsInUseInHost(uuid) && context()->IsBeingBuilt(uuid); On 2016/02/06 at 00:08:39, michaeln ...

4 years, 10 months ago (2016-02-09 00:52:26 UTC) #49

https://codereview.chromium.org/1234813004/diff/580001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/580001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:223: return
IsInUseInHost(uuid) && context()->IsBeingBuilt(uuid);
On 2016/02/06 at 00:08:39, michaeln wrote:
> I don't think this is valid test for whether it's being build by this host
anymore. this worked before when blobreferences could not be passed around prior
to it being fully formed, but thats not tru any more.
> 
> i think can be a one liner now...
>    return async_builder_->IsBeingBuilt(uuid);
> which just checks map_->find(uuid) != map_->end().
> 
> There are two new conditions the old test doesn't work for.
> - this host has a ref to a blob being built elsewhere
> - this host is building it, but has dropped all refs to it

True. Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:139:
MaybeCancelBuildingBlob(uuid, IPCBlobCreationCancelCode::UNKNOWN, context);
On 2016/02/06 at 00:08:39, michaeln wrote:
> moving this early return down below line 146 would eliminate the maybe'ness

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:260:
BlobAsyncTransportRequestBuilder& builder = state->request_builder;
On 2016/02/06 at 00:08:39, michaeln wrote:
> oh,this is different then state->builder!
> maybe call this local request_builder

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:91: void
MaybeCancelBuildingBlob(const std::string& uuid,
On 2016/02/06 at 00:08:39, michaeln wrote:
> I think you remove the 'maybe' from the name, at each callsite, we know its
being built and know we want to cancel it.

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:98: void
ClearAndBreakPendingBlobConstruction(BlobStorageContext* context);
On 2016/02/06 at 00:08:39, michaeln wrote:
> "ClearAndBreak" here has the same meaning as "Cancel" in the previous method
name. I'd vote to be consistent and use the simpler and more concise term in
both places.
> 
> void CancelAll();
> 
> Better yet, it would probably be nice to have context_ be a member. The public
method would no longer need be needed and the dtor could take care of it.

The main issue with that is that context_ would either have to be a weak ptr or
a raw ptr. Neither of these could be initialized on the construction of the
BlobDispatcherHost, as the initialization of BlobStorageContext in the
ChromeBlobStorageContext is actually a posted task, instead of creating it right
away. So this is null initially. So either we have some sort of piping to set
the context in the async builder after it's initialized (not sure the best way
to do this), which would add more complexity, or we just pass the pointer in the
methods it's needed. So I opted for this.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.cc:144:
DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
On 2016/02/06 at 00:08:39, michaeln wrote:
> you could inline these and but move the dchecks in the BlobDataHandleShared
class

? Not sure what you mean. I could remove the shared methods and just call the
context? Done.

The io runner isn't stored in the shared class.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:187:
InvalidateCallbacksAndDone(net::ERR_FAILED, done);
On 2016/02/06 at 00:08:39, michaeln wrote:
> need a return statement here? a test for would be good too

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:105: CHECK(!entry->data.get()) <<
"Blob already constructed: "
On 2016/02/06 at 00:08:39, michaeln wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:166: if (entry->state ==
BlobState::BROKEN) {
On 2016/02/06 at 00:08:39, michaeln wrote:
> maybe simplify to if (state != complete)

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:169: CHECK(entry->state ==
BlobState::COMPLETE);
On 2016/02/06 at 00:08:39, michaeln wrote:
> then theres no need for this check

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:200: // so we return if we haven't
been built yet.
On 2016/02/06 at 00:08:39, michaeln wrote:
> i think this is stale, the intermediary state is not possible, can this be
simplified to return !complete && !broken;

hm, or just state == pending.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:241: void
BlobStorageContext::FlagBlobAsBrokenAndBuild(
On 2016/02/06 at 00:08:39, michaeln wrote:
> please (pretty please) consider renaming these methods using fewer and more
common terms 
> 
> CancelPendingBlob()  or maybe
> BreakPendingBlob() to get that term in there
> 
> Its easier to grok and there are fewer ways to combine the three terms as you
might in a sentence, compared to these six terms. 
> 
> FlagAsBrokenAndBuildBlob()
> 
> Given simple alternatives, the 6 term name is not a good choice.

haha yeah sorry, fixed.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:109: void RegisterBlobUUID(const
std::string& uuid,
On 2016/02/06 at 00:08:39, michaeln wrote:
> harmonize... CreatePendingBlob()

Done.

https://codereview.chromium.org/1234813004/diff/580001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:115: void
FlagBlobAsBrokenAndBuild(const std::string& uuid,
On 2016/02/06 at 00:08:40, michaeln wrote:
> it'd be helpful to readers to rearrange the order to match the order in which
they're to be called.
> 
> CreatePendingBlob()
> CompletePendingBlob()
> CancelPendingBlob() | BreakPendingBlob()
> 
> also the file is large enough that matching order in .h and .cc files would
help

Sounds good.

kinuko

I wasn't able to review much, but this is getting better and better I think... ...

4 years, 10 months ago (2016-02-09 16:42:12 UTC) #50

dmurph

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode77 content/browser/blob_storage/blob_dispatcher_host.cc:77: BlobTransportResult result = StartBuildingBlob(uuid, descriptions); On 2016/02/09 at 16:42:11, ...

4 years, 10 months ago (2016-02-09 23:44:15 UTC) #51

michaeln

haven't made it all the way thru the tests, sending comments i got so far, ...

4 years, 10 months ago (2016-02-09 23:51:01 UTC) #52

haven't made it all the way thru the tests, sending comments i got so far,
there's an interesting case i think we don't handle correctly or have tests for
yet?

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_async_builder_host_unittest.cc:126:
TEST_F(BlobAsyncBuilderHostTest, TestShortcut) {
its not so obvious what shortcut means, a brief doc comment might help

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_async_builder_host_unittest.cc:165:
EXPECT_EQ(0u, host_.blob_building_count());
maybe also test that the number of blobs kept in the central context is
unchanged from before the failed attempt to add another

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:435:
TEST_F(BlobDispatcherHostTest, BlobDestructionEdgeCases) {
Is there a case for FinalReleaseOfBlobBeingBuilt. Since the background thread in
the renderer does not add a ref for the duration of building the blob i think we
can get a final release while being built.

- register id
- start building (or not, the important part is to final relaese with uuid in
the async_builders map)
- dec refcount

then

deletehost or OnStartBuilding or OnMemoryResponse or OnCancelBuilding 

I think there's a couple rough edges to work out there.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:49: // Files are non-const so we
can take the file reference.
ah... i was confused by the sig, would it make sense to pass ownership of the
std::vector<> to make it more clear?

scoped_ptr<std::vector<base::File>>

if you change that one, it would make sense to change all the out vectors to be
treated that way for consistency

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:214: if (entry->state !=
BlobState::COMPLETE) {
the dcheck and the early return are in conflict with one another, one of them
shouldn't be here, also our style guide says "pleeze don't do that"

i think you can leave the dcheck and delete the early return, just document that
it's only valid to call for COMPLETE blobs. that's in keeping with other
decisions about callers of many of these private context methods, the callers
are expected to know what they're doing

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:248: if (!entry) {
I think the only callsite for this method is from within BlobDataHandle. If a
handle exists the underlying blobdata should still exist.  This could probably
be a DCHECK(entry) instead of an early return path for no entry.

That should be fine considering BlobDataHandle also calls DecrementBlobRefCount
upon destruction, and that private context method does not have an early return
for !entry.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:332: DCHECK(src.get());
DCHECK and if are in conflict with one another. Which is it, possible to be null
or not?

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:115: // This will return an empty
snapshot until
the comment got cut off

dmurph

Hello! I did some changes to handle the following cases: * Blob referenced in construction ...

4 years, 10 months ago (2016-02-12 00:46:53 UTC) #53

Hello!

I did some changes to handle the following cases:

* Blob referenced in construction is not done constructing
* Blob refcount is decremented while building (dereferenced in host). We still
need to keep building in case anyone else has a reference (and we will tell the
renderer to release it's memory).

Thing I haven't done yet:
* Verify that the blob uuid set given in description is the same as the blob
uuids in the elements we're given later.

PTAL! Thanks!

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
File content/browser/blob_storage/blob_async_builder_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_async_builder_host_unittest.cc:126:
TEST_F(BlobAsyncBuilderHostTest, TestShortcut) {
On 2016/02/09 at 23:51:01, michaeln wrote:
> its not so obvious what shortcut means, a brief doc comment might help

Done.

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_async_builder_host_unittest.cc:165:
EXPECT_EQ(0u, host_.blob_building_count());
On 2016/02/09 at 23:51:01, michaeln wrote:
> maybe also test that the number of blobs kept in the central context is
unchanged from before the failed attempt to add another

I'm confused. What am I testing?

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:435:
TEST_F(BlobDispatcherHostTest, BlobDestructionEdgeCases) {
On 2016/02/09 at 23:51:01, michaeln wrote:
> Is there a case for FinalReleaseOfBlobBeingBuilt. Since the background thread
in the renderer does not add a ref for the duration of building the blob i think
we can get a final release while being built.
> 
> - register id
> - start building (or not, the important part is to final relaese with uuid in
the async_builders map)
> - dec refcount
> 
> then
> 
> deletehost or OnStartBuilding or OnMemoryResponse or OnCancelBuilding 
> 
> I think there's a couple rough edges to work out there.

Thanks! This helped me find a memory leak, where the async_builder_ was holding
onto a ref of the BlobDispatcherHost, creating a circular dependency.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:49: // Files are non-const so we
can take the file reference.
On 2016/02/09 at 23:51:01, michaeln wrote:
> ah... i was confused by the sig, would it make sense to pass ownership of the
std::vector<> to make it more clear?
> 
> scoped_ptr<std::vector<base::File>>
> 
> if you change that one, it would make sense to change all the out vectors to
be treated that way for consistency

ok sounds good.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:214: if (entry->state !=
BlobState::COMPLETE) {
On 2016/02/09 at 23:51:01, michaeln wrote:
> the dcheck and the early return are in conflict with one another, one of them
shouldn't be here, also our style guide says "pleeze don't do that"
> 
> i think you can leave the dcheck and delete the early return, just document
that it's only valid to call for COMPLETE blobs. that's in keeping with other
decisions about callers of many of these private context methods, the callers
are expected to know what they're doing

I added this to check if it broke stuff, and didn't remove it.  Since people are
calling CreateSnapshot() on the BlobDataHandle willy-nilly in a couple places, I
need to add this constraint in a separate patch.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:248: if (!entry) {
On 2016/02/09 at 23:51:01, michaeln wrote:
> I think the only callsite for this method is from within BlobDataHandle. If a
handle exists the underlying blobdata should still exist.  This could probably
be a DCHECK(entry) instead of an early return path for no entry.
> 
> That should be fine considering BlobDataHandle also calls
DecrementBlobRefCount upon destruction, and that private context method does not
have an early return for !entry.

Done.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:332: DCHECK(src.get());
On 2016/02/09 at 23:51:01, michaeln wrote:
> DCHECK and if are in conflict with one another. Which is it, possible to be
null or not?

I'll remove the DCHECK for now, added a todo.

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/1234813004/diff/640001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:115: // This will return an empty
snapshot until
On 2016/02/09 at 23:51:01, michaeln wrote:
> the comment got cut off

Done.

michaeln

https://codereview.chromium.org/1234813004/diff/730001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/730001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode137 content/browser/blob_storage/blob_dispatcher_host.cc:137: // can correctly break the blob if we're still ...

4 years, 10 months ago (2016-02-17 00:01:10 UTC) #54

dmurph

Hey! So I added a lot of tests in this release, and made any dependency ...

4 years, 10 months ago (2016-02-18 23:35:29 UTC) #55

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 10 months ago (2016-02-19 23:07:13 UTC) #56

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/770001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/770001

4 years, 10 months ago (2016-02-19 23:10:17 UTC) #57

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 10 months ago (2016-02-20 00:10:47 UTC) #58

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/118721)

4 years, 10 months ago (2016-02-20 00:10:49 UTC) #59

michaeln

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/blob_async_builder_host.cc File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/blob_async_builder_host.cc#newcode371 storage/browser/blob/blob_async_builder_host.cc:371: CancelBuildingBlob(uuid, IPCBlobCreationCancelCode::SOURCE_DIED_IN_TRANSIT, On 2016/02/18 23:35:29, dmurph wrote: > On ...

4 years, 10 months ago (2016-02-23 02:25:41 UTC) #60

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:371: CancelBuildingBlob(uuid,
IPCBlobCreationCancelCode::SOURCE_DIED_IN_TRANSIT,
On 2016/02/18 23:35:29, dmurph wrote:
> On 2016/02/17 at 00:01:09, michaeln wrote:
> > tricky... when the building process tries to continue, that shouldn't cause
a
> BAD_IPC msg due ot the blob not being built, what happens in that case now?
> 
> We now ignore memory response messages to blobs that don't exist.

ok

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:389: // synchronous.
On 2016/02/18 23:35:29, dmurph wrote:
> On 2016/02/17 at 00:01:09, michaeln wrote:
> > looks like re-entrancy thru ReferencedBlobFinished would complicate things?
> i'd vote to declare re-entrancy not possible to make it simpler.
> > 
> > as coded, the callbacks will never actually occur syncly
> 
> How does it look now? We don't call this method. I'd... rather protect for
this.
> If this is confusing w/ the manual reference, I can just do two loops instead,
> where the first loop counts them, and the second adds the callbacks.

I don't think you need to protect for that. (If you do you also would want test
cases for it).

Considering the callbacks as being utilized are not synchronous, the comment and
code suggesting they are is confusing. Also, suppose it does actually callback
syncly with a failed reference blob. I think if the loop continued, it  would
crash because state will have been deleted in ReferencedBlobFinished. That's not
actually a problem because that can't actually happen given how things work, but
you'd never know that from looking only at this method impl and reading the
comment here.

https://codereview.chromium.org/1234813004/diff/770001/components/mus/manifes...
File components/mus/manifest.json (right):

https://codereview.chromium.org/1234813004/diff/770001/components/mus/manifes...
components/mus/manifest.json:1: {"url": "mojo://mus/", "name": "Mandoline UI
Services", "capabilities": {}}
picked up a stray changes that don't belong in this cl?

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:73: if (referenced_uuid == uuid
|| context->IsBroken(referenced_uuid)) {
given the test in BDH, probably don't need (refed == uuid) test here

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:79:
handles.push_back(context->GetBlobDataFromUUID(referenced_uuid));
what if this returns null is that ok or a bad ipc or a broken blob?

there's code in the context class that allows it with a squishy comment about
how many it shouldn't allow non-existing blobs as input

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:280:
context->DecrementBlobRefCount(uuid);
now that we have handles, this explicit calls to decrement really looks fishy,
this should not be needed with the handle class

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:283: // So we need to break that
blob.
I think this class still might be unexpectedly re-entrant.

Suppose we have two blobs being built, one references the other (worse yet
suppose a corrupt renderer tells us they reference each other). The cancel call
below will result in a call to ReferencedBlobFinished() and alter our map.

Would this work?

- get vector of handles for each blob still being built
- foreach this->CancelPendingBlob()
- DCHECK(async_blob_map_.empty());

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:63: // 2: we we reference ourself
in the referenced_blob_uuids, or
looks like (1) and (2) are being filtered out by the BDH, also we we (tehe)

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:67: bool RegisterBlobUUID(const
std::string& uuid,
would it help if this method returned a BlobTransportResult as well? So the
various error checks could be more consolidated in this class and you could
handle (1) and (2) in here as well and have a way to indicate badipc.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:137: std::set<std::string>
referenced_blob_uuids,
now that you have a list of handles do we still need the separate list of uuids,
if so why? handles have a uuid() getter

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:141: CHECK(false) << "Blob already
constructed: " << external_builder.uuid();
can this be a dcheck?

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:256: bool error = false;
maybe set *error_code to noerror for safety?

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:323: if (src->IsBroken() ||
src->IsBeingBuilt()) {
IsBeingBuilt() would be very unexpected here, unreachably unexpected, is that
right?

dmurph

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/blob_async_builder_host.cc File storage/browser/blob/blob_async_builder_host.cc (right): https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/blob_async_builder_host.cc#newcode389 storage/browser/blob/blob_async_builder_host.cc:389: // synchronous. On 2016/02/23 at 02:25:40, michaeln wrote: > ...

4 years, 10 months ago (2016-02-23 23:45:32 UTC) #61

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/730001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:389: // synchronous.
On 2016/02/23 at 02:25:40, michaeln wrote:
> On 2016/02/18 23:35:29, dmurph wrote:
> > On 2016/02/17 at 00:01:09, michaeln wrote:
> > > looks like re-entrancy thru ReferencedBlobFinished would complicate
things?
> > i'd vote to declare re-entrancy not possible to make it simpler.
> > > 
> > > as coded, the callbacks will never actually occur syncly
> > 
> > How does it look now? We don't call this method. I'd... rather protect for
this.
> > If this is confusing w/ the manual reference, I can just do two loops
instead,
> > where the first loop counts them, and the second adds the callbacks.
> 
> I don't think you need to protect for that. (If you do you also would want
test cases for it).
> 
> Considering the callbacks as being utilized are not synchronous, the comment
and code suggesting they are is confusing. Also, suppose it does actually
callback syncly with a failed reference blob. I think if the loop continued, it 
would crash because state will have been deleted in ReferencedBlobFinished.
That's not actually a problem because that can't actually happen given how
things work, but you'd never know that from looking only at this method impl and
reading the comment here.

Ok, I'll just declare re-entry not possible and remove this then?

https://codereview.chromium.org/1234813004/diff/770001/components/mus/manifes...
File components/mus/manifest.json (right):

https://codereview.chromium.org/1234813004/diff/770001/components/mus/manifes...
components/mus/manifest.json:1: {"url": "mojo://mus/", "name": "Mandoline UI
Services", "capabilities": {}}
On 2016/02/23 at 02:25:41, michaeln wrote:
> picked up a stray changes that don't belong in this cl?

Whoops, yeah.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:73: if (referenced_uuid == uuid
|| context->IsBroken(referenced_uuid)) {
On 2016/02/23 at 02:25:41, michaeln wrote:
> given the test in BDH, probably don't need (refed == uuid) test here

I removed that check, so it's here and only here now.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:79:
handles.push_back(context->GetBlobDataFromUUID(referenced_uuid));
On 2016/02/23 at 02:25:41, michaeln wrote:
> what if this returns null is that ok or a bad ipc or a broken blob?
> 
> there's code in the context class that allows it with a squishy comment about
how many it shouldn't allow non-existing blobs as input

Question: Would this be a bad ipc, or just cancelling the blob construction?
I'll make it a cancel for now, but it might be a bad ipc. I'm not sure how it
would happen.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:280:
context->DecrementBlobRefCount(uuid);
On 2016/02/23 at 02:25:41, michaeln wrote:
> now that we have handles, this explicit calls to decrement really looks fishy,
this should not be needed with the handle class

Ah, gotcha, this was a mistake.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:283: // So we need to break that
blob.
On 2016/02/23 at 02:25:41, michaeln wrote:
> I think this class still might be unexpectedly re-entrant.
> 
> Suppose we have two blobs being built, one references the other (worse yet
suppose a corrupt renderer tells us they reference each other). The cancel call
below will result in a call to ReferencedBlobFinished() and alter our map.
> 
> Would this work?
> 
> - get vector of handles for each blob still being built
> - foreach this->CancelPendingBlob()
> - DCHECK(async_blob_map_.empty());

Well, what if we clear the state map we cancel them? That means that all of our
ReferencedBlobFinished calls do nothing, as we no longer have stuff in the map.
This way we also don't have to modify the map while iterating it to remove the
non-building items.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:63: // 2: we we reference ourself
in the referenced_blob_uuids, or
On 2016/02/23 at 02:25:41, michaeln wrote:
> looks like (1) and (2) are being filtered out by the BDH, also we we (tehe)

I'll remove that logic from the BDH, and just check it here, like outlined
below.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:67: bool RegisterBlobUUID(const
std::string& uuid,
On 2016/02/23 at 02:25:41, michaeln wrote:
> would it help if this method returned a BlobTransportResult as well? So the
various error checks could be more consolidated in this class and you could
handle (1) and (2) in here as well and have a way to indicate badipc.

Sure, I can do it that way.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:137: std::set<std::string>
referenced_blob_uuids,
On 2016/02/23 at 02:25:41, michaeln wrote:
> now that you have a list of handles do we still need the separate list of
uuids, if so why? handles have a uuid() getter

I use them later on line 132 for validation, so I don't see why I can't keep em
around.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:141: CHECK(false) << "Blob already
constructed: " << external_builder.uuid();
On 2016/02/23 at 02:25:41, michaeln wrote:
> can this be a dcheck?

Sure.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:256: bool error = false;
On 2016/02/23 at 02:25:41, michaeln wrote:
> maybe set *error_code to noerror for safety?

That type doesn't exist. I'll do UNKNOWN.

https://codereview.chromium.org/1234813004/diff/770001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:323: if (src->IsBroken() ||
src->IsBeingBuilt()) {
On 2016/02/23 at 02:25:41, michaeln wrote:
> IsBeingBuilt() would be very unexpected here, unreachably unexpected, is that
right?

It can be reached if someone in browserland is creating blobs and messes up
(i.e. calls AddFinishedBlob with incorrect stuff).

michaeln

this is so close, a couple questions @kinuko, do you want to take another look? ...

4 years, 10 months ago (2016-02-25 01:48:02 UTC) #62

kinuko

Sending some minor comments only. I keep failing to allocate enough time to digest all ...

4 years, 9 months ago (2016-02-25 16:07:33 UTC) #63

Sending some minor comments only.  I keep failing to allocate enough time to
digest all the details, if it starts to look good to michael I'll be probably
happy too, please don't be blocked by my review.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:82: // cancel message back
to the renderer.
weird line breaking

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:101: BlobTransportResult
result = BlobTransportResult::BAD_IPC;
I don't feel initializing this here is making the code any better?  Wouldn't it
be easier to follow (though we need a few more lines) if we just write:

if (uuid.empty()) {
   SendIPCResponse(uuid, BAD_IPC);
   return;
}
...
if (!context->IsBeingBuilt(uuid)) {
  SendIPCResponse(uuid, BAD_IPC);
  return;
}

Result result = async_builder_.StartBuildingBlob(...);
SendIPCResponse(uuid, result);

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:119: // will be greater
than the builder.
nit: it'd be enough to just note that |this| owns async_builder_ so using
base::Unretained(this) is safe

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:132: BlobTransportResult
result = BlobTransportResult::BAD_IPC;
ditto

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:275: return
async_builder_.IsBeingBuilt(uuid);
The term 'InHost' is used in a slightly different meaning here, and it doesn't
feel this method is really helpful.

I think just writing this out like below is easier to follow / simpler... wdyt?

if (async_builder_.IsBeingBuilt(uuid) && !context->IsBeingBuilt(uuid))

https://codereview.chromium.org/1234813004/diff/810001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:128: void
BlobTransportController::CancelBlobTransfer(const std::string& uuid,
I can't spot any callsite for this one, is this necessary?

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:281:
context->CancelPendingBlob(uuid, code);
On 2016/02/25 01:48:02, michaeln wrote:
> Related to the context->HasBlob(uuid) tests recently added to the OnStart and
> OnMemResp, I think this OnCancel msg handling may need defense against that
too.
> A blob being built can be killed unbeknownst to the BlobAsyncBuilderHost but
we
> still have to remove it from the map.
> 
> Wdyt?
> 
> If that's right, i think checking here could take care of that case.
> 
>  if (context->HasBlob(uuid))
>    context->CancelPendingBlob()
>  map.erase(it);
>  

...and if we do this I think we could remove StopBuildingBlob and combine the
two.  (They're pretty confusing)

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:300: // logic.
On 2016/02/25 01:48:02, michaeln wrote:
> // We clear the map before cancelling them to avoid reentrancy.

(nit: the comment about this avoids reentrancy to ReferenceBlobFinished was
helpful for me, I feel some of the original text could be kept as is though
could be still shorten a bit)

dmurph

Thanks guys. I still have to do more investigation on the browsertest failures, they don't ...

4 years, 9 months ago (2016-02-25 23:36:33 UTC) #64

Thanks guys. I still have to do more investigation on the browsertest failures,
they don't make any sense to me. Replied to comments and made changes :)

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:82: // cancel message back
to the renderer.
On 2016/02/25 at 16:07:32, kinuko wrote:
> weird line breaking

Done.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:101: BlobTransportResult
result = BlobTransportResult::BAD_IPC;
On 2016/02/25 at 16:07:32, kinuko wrote:
> I don't feel initializing this here is making the code any better?  Wouldn't
it be easier to follow (though we need a few more lines) if we just write:
> 
> if (uuid.empty()) {
>    SendIPCResponse(uuid, BAD_IPC);
>    return;
> }
> ...
> if (!context->IsBeingBuilt(uuid)) {
>   SendIPCResponse(uuid, BAD_IPC);
>   return;
> }
> 
> Result result = async_builder_.StartBuildingBlob(...);
> SendIPCResponse(uuid, result);

Ah, yes, thanks.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:112: // we decided to break
a blob after RegisterBlobUUID is called.
On 2016/02/25 at 01:48:01, michaeln wrote:
> if IsBeingBuiltInHost(uuid) should builder->CancelBuilding(uuid) to clean up
the builder's map?

We can never have a broken blob and also be in async builder still. The cancel
operation also removes it from the async builder. I can document this and
DCHECK.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:119: // will be greater
than the builder.
On 2016/02/25 at 16:07:32, kinuko wrote:
> nit: it'd be enough to just note that |this| owns async_builder_ so using
base::Unretained(this) is safe

Done.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:132: BlobTransportResult
result = BlobTransportResult::BAD_IPC;
On 2016/02/25 at 16:07:32, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:143: // we decided to break
a blob after sending the memory request.
On 2016/02/25 at 01:48:01, michaeln wrote:
> if IsBeingBuiltInHost(uuid) should builder->CancelBuilding(uuid) to clean up
the builder's map?

We can never be in the async builder if we're broken (or if the entry doesn't
exist in the context). Added comment and dcheck. let me know if I need to add
more.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:159: }
On 2016/02/25 at 01:48:02, michaeln wrote:
> hmmm... what's to ensure that context->HasEntry(uuid) at this point?

Done, added same protection as above.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:275: return
async_builder_.IsBeingBuilt(uuid);
On 2016/02/25 at 16:07:32, kinuko wrote:
> The term 'InHost' is used in a slightly different meaning here, and it doesn't
feel this method is really helpful.
> 
> I think just writing this out like below is easier to follow / simpler...
wdyt?
> 
> if (async_builder_.IsBeingBuilt(uuid) && !context->IsBeingBuilt(uuid))

I agree, done.

https://codereview.chromium.org/1234813004/diff/810001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:128: void
BlobTransportController::CancelBlobTransfer(const std::string& uuid,
On 2016/02/25 at 16:07:32, kinuko wrote:
> I can't spot any callsite for this one, is this necessary?

Not yet, this will be for file failure in the future. I can remove it for now.

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:81: scoped_ptr<BlobDataHandle>
handle =
On 2016/02/25 at 01:48:02, michaeln wrote:
> you might be able to simplify bit to eliminate the duplicated block of code
> 
> handle = context->GetBlobDataFromUUID(referenced_uuid);
> if (!handle || handle->IsBroken()) {
>   context->CancelPendingBlob(...)
>   return BROKEN;
> }

Ah, much better.

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:281:
context->CancelPendingBlob(uuid, code);
On 2016/02/25 at 16:07:32, kinuko wrote:
> On 2016/02/25 01:48:02, michaeln wrote:
> > Related to the context->HasBlob(uuid) tests recently added to the OnStart
and
> > OnMemResp, I think this OnCancel msg handling may need defense against that
too.
> > A blob being built can be killed unbeknownst to the BlobAsyncBuilderHost but
we
> > still have to remove it from the map.
> > 
> > Wdyt?
> > 
> > If that's right, i think checking here could take care of that case.
> > 
> >  if (context->HasBlob(uuid))
> >    context->CancelPendingBlob()
> >  map.erase(it);
> >  
> 
> ...and if we do this I think we could remove StopBuildingBlob and combine the
two.  (They're pretty confusing)

Michael: I added the defense for a renderer who doesn't know the blob is gone in
the OnCancel in BDH. So this is protected.

Both: I can combine this like Kinuko mentioned if that makes it easier.

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:300: // logic.
On 2016/02/25 at 16:07:32, kinuko wrote:
> On 2016/02/25 01:48:02, michaeln wrote:
> > // We clear the map before cancelling them to avoid reentrancy.
> 
> (nit: the comment about this avoids reentrancy to ReferenceBlobFinished was
helpful for me, I feel some of the original text could be kept as is though
could be still shorten a bit)

I made it briefer.

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:322: // properly, unless we have a
wonky renderer.
On 2016/02/25 at 01:48:02, michaeln wrote:
> The asyncbuilder treats !src like a broken src in its RegisterBlobUUID method.
> 
> If possible, I'd vote to have the behavior here match the behavior there and
get rid of the silent error.

Done.

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-02-26 18:47:49 UTC) #65

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/830001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/830001

4 years, 9 months ago (2016-02-26 18:48:37 UTC) #66

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-02-26 20:58:37 UTC) #67

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/180604)

4 years, 9 months ago (2016-02-26 20:58:39 UTC) #68

michaeln

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode143 content/browser/blob_storage/blob_dispatcher_host.cc:143: // we decided to break a blob after sending ...

4 years, 9 months ago (2016-02-26 23:16:38 UTC) #69

dmurph

Thanks! I think this bug might be the cause of the flakiness: https://bugs.chromium.org/p/chromium/issues/detail?id=351753 Michael, I ...

4 years, 9 months ago (2016-03-01 02:45:57 UTC) #70

Thanks!

I think this bug might be the cause of the flakiness:
https://bugs.chromium.org/p/chromium/issues/detail?id=351753

Michael, I might want to have a VC with you about how we should solve this.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:143: // we decided to break
a blob after sending the memory request.
On 2016/02/26 at 23:16:38, michaeln wrote:
> On 2016/02/25 23:36:33, dmurph wrote:
> > On 2016/02/25 at 01:48:01, michaeln wrote:
> > > if IsBeingBuiltInHost(uuid) should builder->CancelBuilding(uuid) to clean
up
> > the builder's map?
> > 
> > We can never be in the async builder if we're broken (or if the entry
doesn't
> > exist in the context). Added comment and dcheck. let me know if I need to
add
> > more.
> 
> Broken no (the only agency that can break it is the builder), but non-existent
is a different story. The building logic does not itself add a ref to the blob
being built. So...
> 
> What's to prevent a rendererA from starting to build a blob, sending a handle
to rendererB, then dropping its reference. The blob still exists and the blob
continues to be built... then rendererB drops its reference... and then
OnMemoryItemResponse() arrives from rendererA.
> 
> I think when that happens we get !entry and IsBeingBultInHost(uuid). If thats
not right, can you explain what does happen in that case?

Ah, I gotcha. So we need to check to see if we're being built, in case it was
fully dereferenced in a different host. Nice, nice, nice catch.

https://codereview.chromium.org/1234813004/diff/810001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:190: if
(IsBeingBuiltInHost(uuid) && !context->IsBeingBuilt(uuid)) {
On 2016/02/26 at 23:16:38, michaeln wrote:
> It might be clearer if the second test were purely an existence test, the
cntx->IsBeingBuilt() could be confusing. The comment explains it well but the
code doesn't match the comment. The convenience method that would help clarify
is cntx->HasBlob(uuid). The following is not possible.
> 
> IsBeingBultInHost(uuid) && cntx->HasBlob(uuid) && !cntx->IsBeingBult()
> 
> but looking at the current test, you might think it is

Done.

dmurph

Alrighty, I've added the child process ref counting, hopefully this works out ;)

4 years, 9 months ago (2016-03-02 19:28:23 UTC) #71

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-03-02 20:51:42 UTC) #72

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/870001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/870001

4 years, 9 months ago (2016-03-02 20:52:14 UTC) #73

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-03-02 21:41:39 UTC) #74

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/189279)

4 years, 9 months ago (2016-03-02 21:41:41 UTC) #75

dmurph

I _believe_ that the most recent patch fixes the failing tests. WOOOOOOO. maybe.

4 years, 9 months ago (2016-03-10 23:41:23 UTC) #76

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-03-11 00:38:26 UTC) #77

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/950001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/950001

4 years, 9 months ago (2016-03-11 00:39:12 UTC) #78

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-03-11 01:18:26 UTC) #79

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/179883)

4 years, 9 months ago (2016-03-11 01:18:28 UTC) #80

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-03-11 21:37:20 UTC) #81

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/950001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/950001

4 years, 9 months ago (2016-03-11 21:37:51 UTC) #82

dmurph

Guys! It's working! The bots are purple because of a clang roll, I'm running another ...

4 years, 9 months ago (2016-03-11 21:37:55 UTC) #83

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-03-11 21:42:15 UTC) #84

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/130027) linux_chromium_chromeos_ozone_rel_ng on ...

4 years, 9 months ago (2016-03-11 21:42:17 UTC) #85

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-03-11 23:44:15 UTC) #86

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/970001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/970001

4 years, 9 months ago (2016-03-11 23:44:39 UTC) #87

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-03-12 00:17:03 UTC) #88

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 9 months ago (2016-03-12 00:17:05 UTC) #89

michaeln

ok, i think this lgtm https://codereview.chromium.org/1234813004/diff/870001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1234813004/diff/870001/content/child/blob_storage/blob_transport_controller.cc#newcode43 content/child/blob_storage/blob_transport_controller.cc:43: // Must be called ...

4 years, 9 months ago (2016-03-14 20:00:19 UTC) #90

dmurph

dmurph@chromium.org changed reviewers: + mpearson@chromium.org

4 years, 9 months ago (2016-03-14 22:39:27 UTC) #91

dmurph

+mpearson for histograms.xml review. Hi Mark! Kinuko, can you PTAL? https://codereview.chromium.org/1234813004/diff/870001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1234813004/diff/870001/content/child/blob_storage/blob_transport_controller.cc#newcode43 ...

4 years, 9 months ago (2016-03-14 22:39:31 UTC) #92

kinuko

My review'd be leaky, but I think I have looked what I had concerned. giving ...

4 years, 9 months ago (2016-03-15 15:29:07 UTC) #93

Mark P

https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/blob_storage_context.cc File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/blob_storage_context.cc#newcode145 storage/browser/blob/blob_storage_context.cc:145: UMA_HISTOGRAM_COUNTS("Storage.Blob.ItemCount", entry->data->items().size()); Judging by this diff, you've refactored some ...

4 years, 9 months ago (2016-03-15 18:36:06 UTC) #94

https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:145:
UMA_HISTOGRAM_COUNTS("Storage.Blob.ItemCount", entry->data->items().size());
Judging by this diff, you've refactored some of this code.  Are you sure the
previous existing histograms are emitted at identical times in this new version
compared to the currently-checked-in version?  Consider edge cases.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:31: enum class
IPCBlobCreationCancelCode {
This enum needs a comment saying that items cannot be reordered or renumbered
and that new items can be added only immediately before LAST.

Also, please use =1, =2, etc. to emphasis that these values should not change.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:37: // The renderer
destructed the blob before it was done trasfering, and there
Who reviewed this file?  "trasfering"  really? :-P  Not one but two typos in one
word and the previous reviewer did not catch it?  (Please tell me this file was
supposed to have been reviewed by someone else.  I am only supposed to be
reviewing the usage of metrics; the change has a whole should've already been
reviewed.)

More typos in the comment below.  Please revise them.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:45: LAST =
REFERENCED_BLOB_BROKEN
Rather than use +1 in the UMA histogram emit line, I tend to prefer END instead
of LAST, and leave it as the final entry (not with the same value as the
previous entry).  It's up to you though.

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49959: +    The reason a blob is broken,
reported only for broken blobs. See
nit: before period add "upon construction" for clarity (if that's true).

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49960: +    Storage.Blob.Broken for the
number of blobs broken.
nit: it's not necessary to see Storage.Blob.Broken for the number of blobs
broken.  You can simply add up these counts.  You probably meant something like
"See Storage.Blob.Broken for the breakdown of blobs broken versus unbroken."

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:60055: +  <int value="1" label="File
write failed"/>
These value don't line up with the C++ enum that I think they're supposed to.

This is a warning flag for me.  Did you actually test the reason values for
failures using about:histograms to see if the recorded failure is what you
expect?

dmurph

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_storage/blob_dispatcher_host.h File content/browser/blob_storage/blob_dispatcher_host.h (right): https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_storage/blob_dispatcher_host.h#newcode1 content/browser/blob_storage/blob_dispatcher_host.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. ...

4 years, 9 months ago (2016-03-15 22:40:15 UTC) #95

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.h (right):

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/03/15 at 15:29:06, kinuko wrote:
> nit: now we're in 2016 =)

Done.

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:74: IPC::TestSink*
sink_;
On 2016/03/15 at 15:29:06, kinuko wrote:
> nit: sink_ could be probably private?

Done.

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:89:
browser_context_(),
On 2016/03/15 at 15:29:06, kinuko wrote:
> nit: do we need these initializers here / can't the default initializers work?

ooops, I had args here, removing them now.

https://codereview.chromium.org/1234813004/diff/990001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:674: // Case 3:
Decrement the refcount while building, after we call
On 2016/03/15 at 15:29:06, kinuko wrote:
> nit: what are case 1 and 2?  (Though I could assume the above two tests would
be the ones)

Yes, the other tests we cases 1 and 2, they were all part of one test and I
split them up.

https://codereview.chromium.org/1234813004/diff/990001/content/browser/fileap...
File content/browser/fileapi/blob_reader_unittest.cc (right):

https://codereview.chromium.org/1234813004/diff/990001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:1157: EXPECT_EQ(0,
memcmp(buffer->data(), "Hello!!!", kDataSize));
On 2016/03/15 at 15:29:06, kinuko wrote:
> "Hello!!!" -> kData ?

Done.

https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/1234813004/diff/990001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:145:
UMA_HISTOGRAM_COUNTS("Storage.Blob.ItemCount", entry->data->items().size());
On 2016/03/15 at 18:36:06, Mark P wrote:
> Judging by this diff, you've refactored some of this code.  Are you sure the
previous existing histograms are emitted at identical times in this new version
compared to the currently-checked-in version?  Consider edge cases.

Yes, this stat this is emitting is identical. I did forget to remove the
ExceededMemory stat, which I can do also in this CL.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:31: enum class
IPCBlobCreationCancelCode {
On 2016/03/15 at 18:36:06, Mark P wrote:
> This enum needs a comment saying that items cannot be reordered or renumbered
and that new items can be added only immediately before LAST.
> 
> Also, please use =1, =2, etc. to emphasis that these values should not change.

Done.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:37: // The renderer
destructed the blob before it was done trasfering, and there
On 2016/03/15 at 18:36:06, Mark P wrote:
> Who reviewed this file?  "trasfering"  really? :-P  Not one but two typos in
one word and the previous reviewer did not catch it?  (Please tell me this file
was supposed to have been reviewed by someone else.  I am only supposed to be
reviewing the usage of metrics; the change has a whole should've already been
reviewed.)
> 
> More typos in the comment below.  Please revise them.

These comments were added pretty recently. Typos fixed.

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:45: LAST =
REFERENCED_BLOB_BROKEN
On 2016/03/15 at 18:36:06, Mark P wrote:
> Rather than use +1 in the UMA histogram emit line, I tend to prefer END
instead of LAST, and leave it as the final entry (not with the same value as the
previous entry).  It's up to you though.

Unfortunately I'm also using this enum in IPCs, which use the the 'last = last
enum value' convention, so I have to do a +1 or a -1 somewhere. Since I'm also
using this enum for switch statements (and not using the default keyword because
that can hide bugs), I'd rather keep it this way so I don't have to add another
switch condition.

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49959: +    The reason a blob is broken,
reported only for broken blobs. See
On 2016/03/15 at 18:36:06, Mark P wrote:
> nit: before period add "upon construction" for clarity (if that's true).

Done.

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49960: +    Storage.Blob.Broken for the
number of blobs broken.
On 2016/03/15 at 18:36:06, Mark P wrote:
> nit: it's not necessary to see Storage.Blob.Broken for the number of blobs
broken.  You can simply add up these counts.  You probably meant something like
"See Storage.Blob.Broken for the breakdown of blobs broken versus unbroken."

Done.

https://codereview.chromium.org/1234813004/diff/990001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:60055: +  <int value="1" label="File
write failed"/>
On 2016/03/15 at 18:36:06, Mark P wrote:
> These value don't line up with the C++ enum that I think they're supposed to.
> 
> This is a warning flag for me.  Did you actually test the reason values for
failures using about:histograms to see if the recorded failure is what you
expect?

Thanks, fixed and verified.

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 9 months ago (2016-03-15 22:40:21 UTC) #96

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/1010001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/1010001

4 years, 9 months ago (2016-03-15 22:40:41 UTC) #97

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 9 months ago (2016-03-15 23:33:44 UTC) #98

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 9 months ago (2016-03-15 23:33:46 UTC) #99

Mark P

https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_storage/blob_storage_constants.h File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/1234813004/diff/990001/storage/common/blob_storage/blob_storage_constants.h#newcode45 storage/common/blob_storage/blob_storage_constants.h:45: LAST = REFERENCED_BLOB_BROKEN On 2016/03/15 22:40:14, dmurph wrote: > ...

4 years, 9 months ago (2016-03-16 19:25:51 UTC) #100

dmurph

Description was changed from ========== [BlobAsync] Hookup patch. This is the final patch that hooks ...

4 years, 8 months ago (2016-03-30 19:18:30 UTC) #101

Description was changed from

==========
[BlobAsync] Hookup patch.
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

Note: Most of the blob_* stuff in the fileapi directory will be moved in
the next patch to the blob_storage directory. I didn't do it here to keep
the patch from getting too complicated.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

to

==========
[BlobAsync] Hookup patch.
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

dmurph

Description was changed from ========== [BlobAsync] Hookup patch. This is the final patch that hooks ...

4 years, 8 months ago (2016-03-30 19:19:13 UTC) #102

Description was changed from

==========
[BlobAsync] Hookup patch.
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

to

==========
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

dmurph

Description was changed from ========== This is the final patch that hooks up the blob ...

4 years, 8 months ago (2016-03-30 19:19:25 UTC) #103

Description was changed from

==========
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

to

==========
[BlobAsync] Asynchronous Blob Construction Final Patch
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

dmurph

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, michaeln@chromium.org, kinuko@chromium.org Link ...

4 years, 8 months ago (2016-03-30 19:21:48 UTC) #105

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/1030001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/1030001

4 years, 8 months ago (2016-03-30 19:22:18 UTC) #106

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 8 months ago (2016-03-30 19:34:13 UTC) #107

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/162158)

4 years, 8 months ago (2016-03-30 19:34:16 UTC) #108

dmurph

https://codereview.chromium.org/1234813004/diff/1010001/storage/common/blob_storage/blob_storage_constants.h File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/1234813004/diff/1010001/storage/common/blob_storage/blob_storage_constants.h#newcode31 storage/common/blob_storage/blob_storage_constants.h:31: // These items cannot be reordered or renumbered. New ...

4 years, 8 months ago (2016-03-30 20:13:26 UTC) #109

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234813004/1030001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234813004/1030001

4 years, 8 months ago (2016-03-30 21:06:43 UTC) #112

commit-bot: I haz the power

Committed patchset #53 (id:1030001)

4 years, 8 months ago (2016-03-30 21:14:44 UTC) #113

commit-bot: I haz the power

Description was changed from ========== [BlobAsync] Asynchronous Blob Construction Final Patch This is the final ...

4 years, 8 months ago (2016-03-30 21:16:03 UTC) #114

Message was sent while issue was closed.

Description was changed from

==========
[BlobAsync] Asynchronous Blob Construction Final Patch
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

to

==========
[BlobAsync] Asynchronous Blob Construction Final Patch
This is the final patch that hooks up the blob storage system to the new
async protocol. In this patch we:
 * Hook up the new classes and modules we created in the previous patches.
 * Remove the old way of creating blobs.
 * Create a few classes like blob_message_filter and blob_dispatcher_host
   for handing IPC messages.

The result of this change makes renderer-initiated blob construction
asynchronous instead of synchronous. So constructing new blobs should be
faster, but the time from construction to read should be the same, as
the reading still has to wait for the blob to be transferred to the
browser.

Because we let the renderer continue before we've sent all of the data,
we use ChildProcess::AddRefProcess() system to keep the renderer alive
while we transfer data.

Patches:
1: https://codereview.chromium.org/1287303002 (committed!)
2: https://codereview.chromium.org/1288373002 (committed!)
3: https://codereview.chromium.org/1292523002 (committed!)
4: https://codereview.chromium.org/1098853003 (committed!)
Hookup: https://codereview.chromium.org/1234813004

BUG=375297

Committed: https://crrev.com/1fb98480c61c563587cbb01de87cfad180fec942
Cr-Commit-Position: refs/heads/master@{#384093}
==========

commit-bot: I haz the power

Patchset 53 (id:??) landed as https://crrev.com/1fb98480c61c563587cbb01de87cfad180fec942 Cr-Commit-Position: refs/heads/master@{#384093}

4 years, 8 months ago (2016-03-30 21:16:07 UTC) #115

Sami

4 years, 8 months ago (2016-03-31 10:52:17 UTC) #116

Message was sent while issue was closed.

This might be unrelated, but one performance benchmark started failing flakily
and complaining about missing blob resources around the time this patch landed.
See https://bugs.chromium.org/p/chromium/issues/detail?id=599416.

Issue 1234813004: [BlobAsync] Asynchronous Blob Construction Final Patch (Closed)