Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc

components/storage_monitor/portable_device_watcher_win.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Any tasks that communicates with the portable device may take >100ms to
 // complete. Those tasks should be run on an blocking thread instead of the
 // UI thread.
 
 #include "components/storage_monitor/portable_device_watcher_win.h"
 
 #include <dbt.h>
 #include <objbase.h>
 #include <portabledevice.h>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/sequenced_worker_pool.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/scoped_propvariant.h"
 #include "components/storage_monitor/removable_device_constants.h"
 #include "components/storage_monitor/storage_info.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace storage_monitor {
 
 namespace {
 
 // Name of the client application that communicates with the MTP device.
 const base::char16 kClientName[] = L"Chromium";
 
 // Name of the sequenced task runner.
 const char kMediaTaskRunnerName[] = "media-task-runner";

[Lei Zhang, 2017/06/19 18:24:46]

I think this needs to be removed too.

[Yeol Park, 2017/06/20 09:04:42]

Done.

 
 // Returns true if |data| represents a class of portable devices.
 bool IsPortableDeviceStructure(LPARAM data) {
   DEV_BROADCAST_HDR* broadcast_hdr =
       reinterpret_cast<DEV_BROADCAST_HDR*>(data);
   if (!broadcast_hdr ||
       (broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE)) {
     return false;
   }
 
@@ skipping 263 matching lines @@
   // the volume name.
   return ((device_name.length() >= 2) && (device_name[1] == L':') &&
       (((device_name[0] >= L'A') && (device_name[0] <= L'Z')) ||
           ((device_name[0] >= L'a') && (device_name[0] <= L'z'))));
 }
 
 // Returns the name of the device specified by |pnp_device_id|.
 base::string16 GetDeviceNameOnBlockingThread(
     IPortableDeviceManager* portable_device_manager,
     const base::string16& pnp_device_id) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[Lei Zhang, 2017/06/19 18:24:46]

Can we retain some kind of DCHECK to help make sure these functions are running
on the right sequence?

[fdoray, 2017/06/19 18:35:02]

DCHECK(media_task_runner_->RunsTasksInCurrentSequence()) would be the correct
way of verifying that tasks run on the right sequence. If you don't have access
to the media_task_runner_, you can still check that the task was posted with the
MayBlock() trait:

base::ThreadRestrictions::AssertIOAllowed();

More details at
https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m...

[Yeol Park, 2017/06/20 09:04:42]

Done.

[Yeol Park, 2017/06/20 09:04:42]

Done.

   DCHECK(portable_device_manager);
   base::string16 name;
   GetFriendlyName(pnp_device_id, portable_device_manager, &name) ||
       GetDeviceDescription(pnp_device_id, portable_device_manager, &name) ||
       GetManufacturerName(pnp_device_id, portable_device_manager, &name);
   return name;
 }
 
 // Access the device and gets the device storage details. On success, returns
 // true and populates |storage_objects| with device storage details.
 bool GetDeviceStorageObjectsOnBlockingThread(
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::StorageObjects* storage_objects) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(storage_objects);
   base::win::ScopedComPtr<IPortableDevice> device;
   if (!SetUp(pnp_device_id, &device))
     return false;
 
   base::string16 device_serial_num;
   if (!GetObjectUniqueId(device.Get(), WPD_DEVICE_OBJECT_ID,
                          &device_serial_num)) {
     return false;
   }
@@ skipping 17 matching lines @@
   return true;
 }
 
 // Accesses the device and gets the device details (name, storage info, etc).
 // On success returns true and fills in |device_details|. On failure, returns
 // false. |pnp_device_id| specifies the plug and play device ID string.
 bool GetDeviceInfoOnBlockingThread(
     IPortableDeviceManager* portable_device_manager,
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::DeviceDetails* device_details) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(portable_device_manager);
   DCHECK(device_details);
   DCHECK(!pnp_device_id.empty());
   device_details->name = GetDeviceNameOnBlockingThread(portable_device_manager,
                                                        pnp_device_id);
   if (IsMassStoragePortableDevice(pnp_device_id, device_details->name))
     return false;
 
   device_details->location = pnp_device_id;
   PortableDeviceWatcherWin::StorageObjects storage_objects;
   return GetDeviceStorageObjectsOnBlockingThread(
       pnp_device_id, &device_details->storage_objects);
 }
 
 // Wrapper function to get an instance of portable device manager. On success,
 // returns true and fills in |portable_device_mgr|. On failure, returns false.
 bool GetPortableDeviceManager(
   base::win::ScopedComPtr<IPortableDeviceManager>* portable_device_mgr) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   HRESULT hr = ::CoCreateInstance(
       __uuidof(PortableDeviceManager), NULL, CLSCTX_INPROC_SERVER,
       IID_PPV_ARGS(portable_device_mgr->GetAddressOf()));
   if (SUCCEEDED(hr))
     return true;
 
   // Either there is no portable device support (Windows XP with old versions of
   // Media Player) or the thread does not have COM initialized.
   DCHECK_NE(CO_E_NOTINITIALIZED, hr);
   return false;
 }
 
 // Enumerates the attached portable devices. On success, returns true and fills
 // in |devices| with the attached portable device details. On failure, returns
 // false.
 bool EnumerateAttachedDevicesOnBlockingThread(
     PortableDeviceWatcherWin::Devices* devices) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(devices);
   base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
   if (!GetPortableDeviceManager(&portable_device_mgr))
     return false;
 
   // Get the total number of devices found on the system.
   DWORD pnp_device_count = 0;
   HRESULT hr = portable_device_mgr->GetDevices(NULL, &pnp_device_count);
   if (FAILED(hr))
     return false;
@@ skipping 14 matching lines @@
   return !devices->empty();
 }
 
 // Handles the device attach event message on a media task runner.
 // |pnp_device_id| specifies the attached plug and play device ID string. On
 // success, returns true and populates |device_details| with device information.
 // On failure, returns false.
 bool HandleDeviceAttachedEventOnBlockingThread(
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::DeviceDetails* device_details) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(device_details);
   base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
   if (!GetPortableDeviceManager(&portable_device_mgr))
     return false;
   // Sometimes, portable device manager doesn't have the new device details.
   // Refresh the manager device list to update its details.
   portable_device_mgr->RefreshDeviceList();
   return GetDeviceInfoOnBlockingThread(portable_device_mgr.Get(), pnp_device_id,
                                        device_details);
 }
@@ skipping 41 matching lines @@
       weak_ptr_factory_(this) {
 }
 
 PortableDeviceWatcherWin::~PortableDeviceWatcherWin() {
   UnregisterDeviceNotification(notifications_);
 }
 
 void PortableDeviceWatcherWin::Init(HWND hwnd) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   notifications_ = RegisterPortableDeviceNotification(hwnd);
-  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
-  media_task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
-      pool->GetNamedSequenceToken(kMediaTaskRunnerName),
-      base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits(

[fdoray, 2017/06/19 18:35:02]

Do tasks posted to this TaskRunner need to run in sequence with other tasks with
the "media-task-runner" sequence token?
https://cs.chromium.org/search/?q=media-task-runner They did before this change.

The recommended way of posting tasks to the same sequence when there isn't a
clear place to own/access the common SequencedTaskRunner is to use
lazy_task_runner.h
https://chromium.googlesource.com/chromium/src/+/0de94189083a49deb78305c2793a...

[Yeol Park, 2017/06/20 09:04:42]

I used LazySequencedTaskRunner instead of SequencedTaskRunner.
If the direction is wrong, let me know.

[fdoray, 2017/06/20 12:15:02]

Previously, |media_task_runner_| was created with the same sequence token as
https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m...
and
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content....
Mutual exclusion was guaranteed between tasks posted to these
SequencedTaskRunner.

With this CL, you no longer have mutual exclusion with these other
SequencedTaskRunners. Is it an issue?

[Yeol Park, 2017/06/21 02:34:32]

Done.

[fdoray, 2017/06/21 12:26:59]

Is it an issue that with your CL, tasks posted to |media_task_runner_| will no
longer run on the same sequence as tasks posted to
https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m...
and
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...
?

+                           {base::MayBlock(), base::TaskPriority::BACKGROUND,
+                            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})
+                           .get();

[fdoray, 2017/06/19 18:35:02]

no .get()

[Yeol Park, 2017/06/20 09:04:42]

Done.

   EnumerateAttachedDevices();
 }
 
 void PortableDeviceWatcherWin::OnWindowMessage(UINT event_type, LPARAM data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!IsPortableDeviceStructure(data))
     return;
 
   base::string16 device_id = GetPnpDeviceId(data);
   if (event_type == DBT_DEVICEARRIVAL)
@@ skipping 150 matching lines @@
     if (storage_notifications_) {
       storage_notifications_->ProcessDetach(
           storage_map_iter->second.device_id());
     }
     storage_map_.erase(storage_map_iter);
   }
   device_map_.erase(device_iter);
 }
 
 }  // namespace storage_monitor