New issue
Advanced search Search tips

Issue 850512 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Simplify Background Fetch Code

Project Member Reported by na...@chromium.org, Jun 7 2018

Issue description

Jotting down the conclusions of a discussion on code simplification with peter@ and rayankans@ here, for Q3:

We're currently maintaining the state for a Background Fetch in multiple components across various layers in code. This makes for a hard to understand, and fragile codebase. Here are the three things we agreed will make this better:

1. Move all knowledge of ongoing fetches (active + pending) to the scheduler. This can be done by moving controllers to it, in one or more containers (one for active fetches, one for pending ones).

2. Make the scheduler an observer of data_manager updates. This decouples the context from the scheduler, and allows the scheduler to automatically pick up any new pending fetches for processing.

3. Make the delegate in chrome layer just a proxy, which allows the download service and offline_items_collection code to talk to the code in the content layer. This simplifies the logic in the delegate and allows it to focus on chrome specific logic.
 
nator_s_Jam.pdf
895 KB Download
Cc: rayankans@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6af2efe5064b16fcb05f41185c958151a38e82cf

commit 6af2efe5064b16fcb05f41185c958151a38e82cf
Author: Peter Beverloo <peter@chromium.org>
Date: Fri Jul 06 17:04:20 2018

Introduce the BackgroundFetchDataManagerObserver class

In the Background Fetch code, there currently isn't a clean separation
between initiating an action to storage (e.g. updating the title of a
background fetch from JavaScript) and propagating such changes to other
subsystems that have to know.

In effect, there are BackgroundFetchJobController instances for
background fetches that aren't active, the BackgroundFetchContext is the
one that owns the instances (as opposed to the BackgroundFetchScheduler)
and much of the control flow serves multiple purposes.

This CL introduces an observer interface for the
BackgroundFetchDataManager that notifies observers about successful
changes to the data store. This means that such code doesn't have to
worry about the source of the change, or whether it was successful.

Right now the BackgroundFetchContext observes the data manager. In the
mid term we'd like the BackgroundFetchScheduler to observe the data
manager instead.

Bug: 850512
Change-Id: Ie4d5b280ac1f220553cc2867c035bf4662c4e88f
Reviewed-on: https://chromium-review.googlesource.com/1127784
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573002}
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_context.h
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_data_manager.h
[add] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_data_manager_observer.h
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/6af2efe5064b16fcb05f41185c958151a38e82cf/content/browser/background_fetch/storage/update_registration_ui_task.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96544636e959581575b9c4f1ac2ecd1c5f40296d

commit 96544636e959581575b9c4f1ac2ecd1c5f40296d
Author: Rayan Kanso <rayankans@chromium.org>
Date: Thu Jul 12 16:31:27 2018

[Background Fetch] Add DB corruption handler to data manager observer.

Bug: 850512
Change-Id: Iddd2773f63fd7aa0368e980a347c1bef4312f23d
Reviewed-on: https://chromium-review.googlesource.com/1128839
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574593}
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_context.h
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_data_manager.h
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_data_manager_observer.h
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/background_fetch_test_data_manager.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/database_task.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/database_task.h
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/delete_registration_task.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/get_settled_fetches_task.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/mark_registration_for_deletion_task.cc
[modify] https://crrev.com/96544636e959581575b9c4f1ac2ecd1c5f40296d/content/browser/background_fetch/storage/start_next_pending_request_task.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87886930a2bd83676885342a3cd19665613cdc07

commit 87886930a2bd83676885342a3cd19665613cdc07
Author: Peter Beverloo <peter@chromium.org>
Date: Wed Jul 25 15:34:17 2018

Move Background Fetch registration creation to the Data Manager observer

When a Background Fetch registration is created, a BGFetchJobContoller
is created immediately after committing it to storage even if the job
can't be started just yet -- this should be the scheduler's decision.

This CL is the first step in removing the assumption that each existing
Background Fetch registration is represented by a BGFetchJobController.
Acknowledging creation of the registration is separated from indicating
that a new registration is available, by adding an OnRegistrationCreated
method to the BackgroundFetchDataManagerObserver.

As part of this refactoring, in an effort to avoid passing around unique
pointers to BackgroundFetchRegistration objects, these are now passed by
const& where the receiver makes a copy when necessary. This enabled
reducing knowledge of the protobufs throughout our system, which is a
nice clean-up on its own.

TBR=avi for BUILD.gn

Bug: 850512
Change-Id: I8d3c42a11ab89f7b7c3573a6f9a21e096493b838
Reviewed-on: https://chromium-review.googlesource.com/1149871
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577903}
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/BUILD.gn
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_context.h
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_data_manager.h
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_data_manager_observer.h
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/background_fetch_service_unittest.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/create_metadata_task.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/create_metadata_task.h
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/database_helpers.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/database_helpers.h
[add] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/get_registration_task.cc
[add] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/get_registration_task.h
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/start_next_pending_request_task.cc
[modify] https://crrev.com/87886930a2bd83676885342a3cd19665613cdc07/content/browser/background_fetch/storage/start_next_pending_request_task.h

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c41426cdef01aa2418fb4c5bbe5df3cad27c552

commit 8c41426cdef01aa2418fb4c5bbe5df3cad27c552
Author: Peter Beverloo <peter@chromium.org>
Date: Fri Aug 10 16:23:33 2018

Remove Background Fetch' scheduler's ability to parallelize

We're not using this code in production. It's hardly tested, has an odd
sequencing order and brings a lot of complication that we can remove for
now. It's also the cause of a number of crashes.

Bug: 850512
Change-Id: I33050f54dfbafa9192390d1ad5f3ed34cce91aca
Reviewed-on: https://chromium-review.googlesource.com/1165554
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582192}
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/BUILD.gn
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_context.h
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_data_manager.h
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_job_controller.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_job_controller.h
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_job_controller_unittest.cc
[delete] https://crrev.com/2cb9efe882557d814a9973cd8c0448831f36fc30/content/browser/background_fetch/background_fetch_request_manager.h
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_scheduler.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_scheduler.h
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/background_fetch_scheduler_unittest.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/storage/mark_request_complete_task.cc
[modify] https://crrev.com/8c41426cdef01aa2418fb4c5bbe5df3cad27c552/content/browser/background_fetch/storage/mark_request_complete_task.h

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bfe9be8aef27c0713748ee112870d8abc3e59a6

commit 1bfe9be8aef27c0713748ee112870d8abc3e59a6
Author: Rayan Kanso <rayankans@chromium.org>
Date: Mon Nov 05 21:42:46 2018

[Background Fetch] Move ownership of job controllers to scheduler

Major changes:
- The scheduler owns the job controllers. It is also the data manager
/ service worker observer now instead of BackgroundFetchContext.
- The job controller itself is in charge of processing a fetch. This
simplifies the `abort` workflow thanks to weak_ptrs. The scheduler can
still be extended to process multiple requests though.
- The scheduler cleanup process for a job controller handles the
registration cleanup and event dispatching.
- The UpdateUI DB tasks and checks and associated checks were removed
since the UI can only be updated once when the registration
fails/succeeds.
- Scheduler::Controller was removed since it doesn't make sense to have
anymore now that the scheduler directly owns the controllers (also
RequestProvider because it added no value).
- There were some other minor readability cleanups along the way.

Bug: 850512
Change-Id: Icff68da7ccc20d8112d283843dd2ad6c6d1a9a26
Reviewed-on: https://chromium-review.googlesource.com/c/1297412
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605475}
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/BUILD.gn
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_context.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_data_manager.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_data_manager_observer.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_delegate_proxy.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_delegate_proxy.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_delegate_proxy_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_event_dispatcher.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_event_dispatcher.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_job_controller.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_job_controller.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_job_controller_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_metrics.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_metrics.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_scheduler.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_scheduler.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_scheduler_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_service_impl.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/background_fetch_service_unittest.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/storage/mark_request_complete_task.cc
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/content/browser/background_fetch/storage/start_next_pending_request_task.cc
[delete] https://crrev.com/87f76187bc214be9d10a69aa4d3681e543c5e312/content/browser/background_fetch/storage/update_registration_ui_task.cc
[delete] https://crrev.com/87f76187bc214be9d10a69aa4d3681e543c5e312/content/browser/background_fetch/storage/update_registration_ui_task.h
[modify] https://crrev.com/1bfe9be8aef27c0713748ee112870d8abc3e59a6/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76e240cca7d352f990835f2f414a7243ef623d36

commit 76e240cca7d352f990835f2f414a7243ef623d36
Author: Rayan Kanso <rayankans@google.com>
Date: Wed Nov 07 13:30:19 2018

[Background Fetch] Cleanup after large refactor.

Few more minor changes that didn't make it in cr/1297412.

Bug: 850512
Change-Id: I7a1070135aa4d10a238c8c12fd4a95ffeac34343
Reviewed-on: https://chromium-review.googlesource.com/c/1320130
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606026}
[modify] https://crrev.com/76e240cca7d352f990835f2f414a7243ef623d36/content/browser/background_fetch/background_fetch_context.cc
[modify] https://crrev.com/76e240cca7d352f990835f2f414a7243ef623d36/content/browser/background_fetch/background_fetch_data_manager.cc
[modify] https://crrev.com/76e240cca7d352f990835f2f414a7243ef623d36/content/browser/background_fetch/background_fetch_scheduler.cc

Sign in to add a comment