Allow BlobDataHandles to be copied, and have their UUIDs read, on any thread.

Allow BlobDataHandles to be copied, and have their UUIDs read, on any thread.

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

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

[michaeln, 2014-04-28 18:25:22]

lgtm (modulo small things to look at)

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_data_handle.cc (left):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_data_handle.cc:48: void BlobDataHandle::DeleteHelper(
nice that this is no longer needed since the Shared class carries the ref to
context

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_data_handle.cc:62: BlobData* BlobDataHandle::data()
const {
if the DCHECK(io_task_runner_->RunsTasksOnCurrentThread());= is here instead of
in the Shared class, maybe it doesn't need the io_task_runner_ at all?

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_data_handle.h (right):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_data_handle.h:38: class BlobDataHandleShared
too bad this has to be in the .h file :(

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_context.h:32: class BlobDataHandleShared;
this doesn't look meaningful since it's really an inner class, is this needed?

[ericu, 2014-04-28 18:43:01]

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_data_handle.cc:62: BlobData* BlobDataHandle::data()
const {
On 2014/04/28 18:25:22, michaeln wrote:
> if the DCHECK(io_task_runner_->RunsTasksOnCurrentThread());= is here instead
of
> in the Shared class, maybe it doesn't need the io_task_runner_ at all?

Good point; done.

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/259773006/diff/60001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_context.h:32: class BlobDataHandleShared;
On 2014/04/28 18:25:22, michaeln wrote:
> this doesn't look meaningful since it's really an inner class, is this needed?

Done.

[ericu, 2014-04-28 19:45:09]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:45:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-28 21:02:37]

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

[commit-bot: I haz the power, 2014-04-28 21:02:37]

Try jobs failed on following builders:
  tryserver.chromium on ios_dbg_simulator

[ericu, 2014-04-28 21:22:03]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:22:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-28 21:58:16]

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

[commit-bot: I haz the power, 2014-04-28 21:58:16]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[ericu, 2014-04-28 22:27:12]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:29:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-28 23:12:18]

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

[commit-bot: I haz the power, 2014-04-28 23:12:19]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[ericu, 2014-04-28 23:13:29]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-29 00:02:42]

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

[commit-bot: I haz the power, 2014-04-29 00:02:42]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[ericu, 2014-04-29 00:07:35]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-29 02:10:50]

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

[commit-bot: I haz the power, 2014-04-29 02:10:51]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[ericu, 2014-04-29 03:26:12]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:26:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/80001

[commit-bot: I haz the power, 2014-04-29 09:57:12]

Change committed as 266817

[Michael Achenbach, 2014-04-29 10:57:21]

A revert of this CL has been created in
https://codereview.chromium.org/252163002/ by machenbach@chromium.org.

The reason for reverting is: Causes leaks:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[ericu, 2014-04-30 17:13:15]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:17:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/100001

[commit-bot: I haz the power, 2014-04-30 17:29:51]

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

[commit-bot: I haz the power, 2014-04-30 17:29:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[ericu, 2014-04-30 17:39:08]

Arg--need an additional OWNER review for the test fix.
Antoine, can you take a quick look at
content/browser/loader/upload_data_stream_builder_unittest.cc?

Thanks!

[piman, 2014-04-30 22:31:41]

MessageLoop::RunUntilIdle is deprecated, suggesting using a RunLoop instead.
Mind replacing the pattern?

[ericu, 2014-04-30 22:51:49]

On 2014/04/30 22:31:41, piman wrote:
> MessageLoop::RunUntilIdle is deprecated, suggesting using a RunLoop instead.
> Mind replacing the pattern?

Done.

[piman, 2014-04-30 23:23:27]

LGTM, thanks!

[ericu, 2014-04-30 23:31:15]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-30 23:32:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/259773006/160001

[commit-bot: I haz the power, 2014-05-01 04:22:23]

Change committed as 267423

[falken, 2014-05-01 07:43:56]

On 2014/05/01 04:22:23, I haz the power (commit-bot) wrote:
> Change committed as 267423

It looks like this broke ASAN bot:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

[falken, 2014-05-01 07:56:59]

A revert of this CL has been created in
https://codereview.chromium.org/261683005/ by falken@chromium.org.

The reason for reverting is: I'm sorry to revert this change. It looks like it
breaks the ASAN bot:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

Direct leak of 120 byte(s) in 3 object(s) allocated from:
    #0 0x520dbb in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
    #1 0x3a0f611 in
webkit_blob::BlobDataHandle::BlobDataHandle(webkit_blob::BlobData*,
webkit_blob::BlobStorageContext*, base::SequencedTaskRunner*)
webkit/browser/blob/blob_data_handle.cc:42
    #2 0x3a0fd71 in
webkit_blob::BlobStorageContext::GetBlobDataFromUUID(std::string const&)
webkit/browser/blob/blob_storage_context.cc:69
    #3 0x268955d in ResolveBlobReference
content/browser/loader/upload_data_stream_builder.cc:73
....

[michaeln, 2014-05-01 18:50:47]

https://codereview.chromium.org/259773006/diff/160001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder_unittest.cc (right):

https://codereview.chromium.org/259773006/diff/160001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder_unittest.cc:264:
handle2.reset();
bah... also need to reset request_body and maybe upload, might be cleaner and
more maintainable to push the majority of the method body down a scope...

void ResolveBlobAndCreateUploadDataStream() {
  base::MessageLoop message_loop;
  {
    stuff
  }
  base::RunLoop().RunUntilIdle();
}

https://codereview.chromium.org/259773006/diff/160001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder_unittest.cc:266:
run_loop.RunUntilIdle();
i've seen this more consisely written as...
base::RunLoop().RunUntilIdle();
...in other tests.

[michaeln, 2014-05-03 00:38:50]

Putting thru the cq again as https://codereview.chromium.org/261993004/ with the
fix described earlier.