[Background Fetch] Cleanup/fix thread safety

[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:

- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.

- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.

- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.

- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since
  those callbacks are sometimes stored by objects that are themselves
  owned by the context, and so there used to be reference cycles when
  those callbacks held strong references to the context; this used to
  cause us to leak the context sometimes.

- Adds comments for clarity in tests which use StoragePartition to get
  a URLRequestContext, but then construct their own
  BackgroundFetchContext etc instead of using the one owned by the
  StoragePartition.

BUG=none

Review-Url: https://codereview.chromium.org/2973233002
Cr-Commit-Position: refs/heads/master@{#485639}
Committed: https://chromium.googlesource.com/chromium/src/+/57400c18f9da4828e4b59c33db37cd2b2f52e36f

[johnme, 2017-07-07 16:08:19]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2017-07-10 13:13:22]

lgtm, but please wait for Dan's review

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:212:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
ChromeBlobStorageContext::CreateFileBackedBlob() will DCHECK, so what about
adding a comment on line 254 to indicate this? This is the sort of line that
we'd forget about when refactoring this method (which is bound to happen).

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:71:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Please add a DCHECK_CURRENTLY_ON to the constructor as well, since that'll
happen on UI. Would be worthwhile to document its threading model in the header
file too.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_request_info.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.cc:21:
base::SequencedTaskRunnerHandle::Get()),
Should we be more explicit and bind to
BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)?

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.cc:55:
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
ugh. noise. so glad that we'll be able to get rid of all of this once the
ClientProxy thing is in place.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_test_base.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_test_base.cc:204: //
TODO(johnme): Make this REAL_IO_THREAD to get coverage for thread safety.
Is that ever going to happen? OK if you plan to work on this, otherwise please
remove it.

https://codereview.chromium.org/2973233002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/chrome_blob_storage_context.h:53: // Unlike the
others, this method must be called on the UI thread.
// Must be called on the UI thread.

(The first bit is implied by the class-level comment.)

[Dan Elphick, 2017-07-10 13:26:01]

delphick@chromium.org changed reviewers:
+ delphick@chromium.org

[Dan Elphick, 2017-07-10 13:26:02]

lgtm

[johnme, 2017-07-10 13:41:08]

Thanks, addressed review comments.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:212:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> ChromeBlobStorageContext::CreateFileBackedBlob() will DCHECK, so what about
> adding a comment on line 254 to indicate this? This is the sort of line that
> we'd forget about when refactoring this method (which is bound to happen).

Done.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:71:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> Please add a DCHECK_CURRENTLY_ON to the constructor as well, since that'll
> happen on UI. Would be worthwhile to document its threading model in the
header
> file too.

Done here; the header already says "Must only be used on the IO thread."

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_request_info.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.cc:21:
base::SequencedTaskRunnerHandle::Get()),
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> Should we be more explicit and bind to
> BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)?

No, we don't actually care what thread we're deleted on, we just care that it's
the same thread as the one that we're accessed on.

The sequence_checker_ is automatically bound to the current sequence in the
constructor since I don't call DETACH_FROM_SEQUENCE, so it's cleanest to do the
same thing for RefCountedDeleteOnSequence.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.cc:55:
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> ugh. noise. so glad that we'll be able to get rid of all of this once the
> ClientProxy thing is in place.

Acknowledged.

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_test_base.cc (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_test_base.cc:204: //
TODO(johnme): Make this REAL_IO_THREAD to get coverage for thread safety.
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> Is that ever going to happen? OK if you plan to work on this, otherwise please
> remove it.

Changed this to explain why it's not REAL_IO_THREAD, rather than being a TODO.

https://codereview.chromium.org/2973233002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2973233002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/chrome_blob_storage_context.h:53: // Unlike the
others, this method must be called on the UI thread.
On 2017/07/10 13:13:21, Peter Beverloo wrote:
> // Must be called on the UI thread.
> 
> (The first bit is implied by the class-level comment.)

Done.

[johnme, 2017-07-10 13:43:11]

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

[commit-bot: I haz the power, 2017-07-10 13:43:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 16:39:15]

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

[commit-bot: I haz the power, 2017-07-10 16:39:16]

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

[johnme, 2017-07-10 17:49:21]

Description was changed from

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:
- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.
- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.
- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.

BUG=none
==========

to

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:
- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.
- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.
- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.
- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since those
callbacks are sometimes stored by objects that are themselves owned by the
context, and so there used to be reference cycles when those callbacks held
strong references to the context; this used to cause us to leak the context
sometimes.
- Adds comments for clarity in tests which use StoragePartition to get a
URLRequestContext, but then construct their own BackgroundFetchContext etc
instead of using the one owned by the StoragePartition.

BUG=none
==========

[johnme, 2017-07-10 17:49:37]

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

[commit-bot: I haz the power, 2017-07-10 17:49:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2017-07-10 17:51:10]

johnme@chromium.org changed reviewers:
+ avi@chromium.org

[johnme, 2017-07-10 17:51:10]

+avi: Please review content/browser/blob_storage/ and
content/browser/storage_partition_impl.cc. Thanks!

[Avi (use Gerrit), 2017-07-10 18:07:11]

content stuff lgtm stamp

[commit-bot: I haz the power, 2017-07-10 19:57:46]

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

[commit-bot: I haz the power, 2017-07-10 19:57:48]

Dry run: This issue passed the CQ dry run.

[johnme, 2017-07-11 11:28:20]

Description was changed from

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:
- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.
- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.
- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.
- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since those
callbacks are sometimes stored by objects that are themselves owned by the
context, and so there used to be reference cycles when those callbacks held
strong references to the context; this used to cause us to leak the context
sometimes.
- Adds comments for clarity in tests which use StoragePartition to get a
URLRequestContext, but then construct their own BackgroundFetchContext etc
instead of using the one owned by the StoragePartition.

BUG=none
==========

to

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:

- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.

- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.

- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.

- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since
  those callbacks are sometimes stored by objects that are themselves
  owned by the context, and so there used to be reference cycles when
  those callbacks held strong references to the context; this used to
  cause us to leak the context sometimes.

- Adds comments for clarity in tests which use StoragePartition to get
  a URLRequestContext, but then construct their own
  BackgroundFetchContext etc instead of using the one owned by the
  StoragePartition.

BUG=none
==========

[johnme, 2017-07-11 11:39:10]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-07-11 11:39:11]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
delphick@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2973233002/#ps60001
(title: "Remove n.b. from comments")

[commit-bot: I haz the power, 2017-07-11 11:39:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 12:50:39]

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

[commit-bot: I haz the power, 2017-07-11 12:50:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2017-07-11 15:09:00]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2017-07-11 15:09:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 15:56:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499785740100270,
"parent_rev": "86da53572bd116369686d4727dad2d4fb6f07176", "commit_rev":
"57400c18f9da4828e4b59c33db37cd2b2f52e36f"}

[commit-bot: I haz the power, 2017-07-11 15:56:53]

Description was changed from

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:

- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.

- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.

- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.

- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since
  those callbacks are sometimes stored by objects that are themselves
  owned by the context, and so there used to be reference cycles when
  those callbacks held strong references to the context; this used to
  cause us to leak the context sometimes.

- Adds comments for clarity in tests which use StoragePartition to get
  a URLRequestContext, but then construct their own
  BackgroundFetchContext etc instead of using the one owned by the
  StoragePartition.

BUG=none
==========

to

==========
[Background Fetch] Cleanup/fix thread safety

Document, cleanup, and in several cases fix thread safety of Background
Fetch code. In particular:

- BackgroundFetchContext becomes DeleteOnIOThread to simplify shutdown.

- BackgroundFetchRequestInfo becomes RefCountedDeleteOnSequence to fix
  it being deleted on a different thread to the one it is accessed on.

- BackgroundFetchRequestInfo::SetDownloadStatePopulated and
  SetResponseDataPopulated are split out and required to be called on
  the IO thread, to fix BackgroundFetchJobController writing these
  fields on the UI thread potentially in parallel with them being read
  on the IO thread.

- Uses WeakPtrs when binding callbacks to BackgroundFetchContext, since
  those callbacks are sometimes stored by objects that are themselves
  owned by the context, and so there used to be reference cycles when
  those callbacks held strong references to the context; this used to
  cause us to leak the context sometimes.

- Adds comments for clarity in tests which use StoragePartition to get
  a URLRequestContext, but then construct their own
  BackgroundFetchContext etc instead of using the one owned by the
  StoragePartition.

BUG=none

Review-Url: https://codereview.chromium.org/2973233002
Cr-Commit-Position: refs/heads/master@{#485639}
Committed:
https://chromium.googlesource.com/chromium/src/+/57400c18f9da4828e4b59c33db37...
==========

[commit-bot: I haz the power, 2017-07-11 15:56:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/57400c18f9da4828e4b59c33db37...