Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11358243:
Redesigned and refactored ScopedMTPDeviceMapEntry, MTPDeviceMapService & MTPDeviceDelegate classes. (Closed)
|
Created:
8 years, 1 month ago by kmadhusu Modified:
8 years ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRedesigned and refactored MTPDeviceMapService and MTPDeviceDelegate.
Previously, MTPDeviceDelegate classes were ref-counted. Since the MTPDeviceDelegate was ref-counted, the design was complicated and the ownership of the MTPDeviceDelegateImpl object was not clearly defined.
In this CL,
(1) MTPDeviceDelegate supports weak pointers and is not ref-counted.
(2) MediaFileSystemRegistry manages ScopedMTPDeviceMapEntry object.
(3) ScopedMTPDeviceMapEntry manages the lifetime of MTPDeviceDelegateImpl object via MTPDeviceMapService class.
(4) ScopedMTPDeviceMapEntry adds an entry in MTPDeviceMapService when the MTP device is registered as a media gallery file system by an extension.
(5) ScopedMTPDeviceMapEntry removes an entry from MTPDeviceMapService, when the browser is in shutdown mode or when the last extension using the device delegate is destroyed or when the device is detached from the system or when the user revoke the device gallery permissions.
(7) FileSystemOperationContext stores a weak pointer of MTPDeviceDelegate object.
(8) DeviceMediaFileUtil validates the weak pointer before forwarding any file operation request to the MTPDeviceDelegateImpl.
(9) When an entry is removed from MTPDeviceMapService, the ownership of the |MTPDeviceDelegateImpl| is handed off to the |MTPDeviceDelegateImpl| object which takes care of deleting itself on the right thread.
BUG=144527
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169991
Patch Set 1 : '' #
Total comments: 8
Patch Set 2 : Addressed comments #Patch Set 3 : Rebase #Patch Set 4 : Fix unit test #
Total comments: 38
Patch Set 5 : Addressed review comments. #
Total comments: 2
Patch Set 6 : Addressed review comments #
Total comments: 5
Patch Set 7 : Fixed comment. #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 2
Patch Set 11 : Fixed GetFileInfoWorker #Messages
Total messages: 29 (0 generated)
thestig: Please review media gallery code. kinuko: Please review webkit/fileapi code. Thanks.
I'm afraid this doesn't work in the current architecture since FileSystemOperationContext may access the delegate on FILE thread too. > (7) FileSystemOperationContext stores a weak pointer of MTPDeviceDelegate object. Depending on this refactoring's importance, at this point it might be better to start refactoring the current synchronous MTP file util code to async operation? I had recommended the current architecture since at the time I assumed most of actual file operations would be implemented in an blocking manner but the current code basically dispatches the task to UI thread. In this way the reliance on yet another blocking thread (in addition to IO+UI) is making the code probably more complex. http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_system... File webkit/fileapi/file_system_operation_context.h (right): http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_system... webkit/fileapi/file_system_operation_context.h:46: base::WeakPtr<MTPDeviceDelegate> mtp_device_delegate() const { I'm afraid this weakptr is created on IO thread but may be accessed on FILE thread. http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devic... File webkit/fileapi/media/device_media_file_util.cc (right): http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devic... webkit/fileapi/media/device_media_file_util.cc:171: return base::PLATFORM_FILE_ERROR_FAILED; return base::PLATFORM_FILE_ERROR_NOT_FOUND ? http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_d... File webkit/fileapi/media/mtp_device_delegate.h (right): http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_d... webkit/fileapi/media/mtp_device_delegate.h:60: // deleteting itself on the right thread. This function should cancel all the nit: deleteing -> deleting http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_d... File webkit/fileapi/media/mtp_device_map_service.cc (right): http://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_d... webkit/fileapi/media/mtp_device_map_service.cc:43: GetMTPDeviceDelegate(const std::string& filesystem_id) { nit: this new indentation looks a bit unusual...?
kinuko@: I am all in to convert the synchronous file util functions to asynchronous functions. That will reduce the complexity of MTPDeviceDelegateImpl class. I guess, that change is independent of this refactoring. Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_syste... File webkit/fileapi/file_system_operation_context.h (right): https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_syste... webkit/fileapi/file_system_operation_context.h:46: base::WeakPtr<MTPDeviceDelegate> mtp_device_delegate() const { On 2012/11/15 10:23:39, kinuko wrote: > I'm afraid this weakptr is created on IO thread but may be accessed on FILE > thread. Can you please provide more details on how this can be accessed on FILE thread? When I tested the code, I didn't not hit any invalid thread access error. WeakPtr thread checker seems to be happy. As you said, I create the WeakPtr @line 43. "delegate->GetAsWeakPtrOnIOThread()" detaches the weak pointer from IO thread. I thought we dereference this weak pointer only in device_media_file_util which is called on media sequenced task runner thread. https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_file_util.cc (right): https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_file_util.cc:171: return base::PLATFORM_FILE_ERROR_FAILED; On 2012/11/15 10:23:39, kinuko wrote: > return base::PLATFORM_FILE_ERROR_NOT_FOUND ? Done. https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_delegate.h (right): https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:60: // deleteting itself on the right thread. This function should cancel all the On 2012/11/15 10:23:39, kinuko wrote: > nit: deleteing -> deleting Done. https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_map_service.cc (right): https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.cc:43: GetMTPDeviceDelegate(const std::string& filesystem_id) { On 2012/11/15 10:23:39, kinuko wrote: > nit: this new indentation looks a bit unusual...? Fixed.
On 2012/11/15 17:52:23, kmadhusu wrote: > kinuko@: I am all in to convert the synchronous file util functions to > asynchronous functions. That will reduce the complexity of MTPDeviceDelegateImpl > class. This sounds great! > I guess, that change is independent of this refactoring. > > Addressed review comments. PTAL. > Thanks. > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_syste... > File webkit/fileapi/file_system_operation_context.h (right): > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/file_syste... > webkit/fileapi/file_system_operation_context.h:46: > base::WeakPtr<MTPDeviceDelegate> mtp_device_delegate() const { > On 2012/11/15 10:23:39, kinuko wrote: > > I'm afraid this weakptr is created on IO thread but may be accessed on FILE > > thread. > > Can you please provide more details on how this can be accessed on FILE thread? > > When I tested the code, I didn't not hit any invalid thread access error. > WeakPtr thread checker seems to be happy. As you said, I create the WeakPtr > @line 43. "delegate->GetAsWeakPtrOnIOThread()" detaches the weak pointer from IO > thread. I thought we dereference this weak pointer only in > device_media_file_util which is called on media sequenced task runner thread. Sorry I meant the media sequenced task runner but not FILE thread. And ok I found that you're deleting the delegate on the same sequenced task runner... the name AsWeakPtrOnIOThread has confused me. Can you add some comment (like the ones in delegate code) too so that the readers can be sure that the weak ptr is handled only on the Media task runner after this. > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devi... > File webkit/fileapi/media/device_media_file_util.cc (right): > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/devi... > webkit/fileapi/media/device_media_file_util.cc:171: return > base::PLATFORM_FILE_ERROR_FAILED; > On 2012/11/15 10:23:39, kinuko wrote: > > return base::PLATFORM_FILE_ERROR_NOT_FOUND ? > > Done. > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... > File webkit/fileapi/media/mtp_device_delegate.h (right): > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... > webkit/fileapi/media/mtp_device_delegate.h:60: // deleteting itself on the right > thread. This function should cancel all the > On 2012/11/15 10:23:39, kinuko wrote: > > nit: deleteing -> deleting > > Done. > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... > File webkit/fileapi/media/mtp_device_map_service.cc (right): > > https://codereview.chromium.org/11358243/diff/12011/webkit/fileapi/media/mtp_... > webkit/fileapi/media/mtp_device_map_service.cc:43: GetMTPDeviceDelegate(const > std::string& filesystem_id) { > On 2012/11/15 10:23:39, kinuko wrote: > > nit: this new indentation looks a bit unusual...? > > Fixed.
kinuko@: Added more comments in FileSystemOperationContext regarding |mtp_device_delegate_|. thestig@: CL is ready for review. Please review media gallery changes. Thanks.
webkit/fileapi lgtm http://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_galle... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h (right): http://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_galle... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:30: // supports weak pointers because fileapi::MTPDeviceDelegate supports weak nit: fileapi::MTPDeviceDelegate -> 'the base class' might make it slightly easier to follow (for me) http://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_galle... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:40: // MTPDeviceDelegate: nit: MTPDeviceDelegate overrides ? http://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devic... File webkit/fileapi/media/device_media_file_util.cc (right): http://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devic... webkit/fileapi/media/device_media_file_util.cc:87: .PassAs<FileSystemFileUtil::AbstractFileEnumerator>(); nit: hmm... if we're just creating a new scoped_ptr (but not passing rvalues) just using scoped_ptr<foo>(new bar) would look simpler. scoped_ptr<FileSystemFileUtil::AbstractFileEnumerator>(new ...)
https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it possible for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can store a pointer to CancellationFlag and check that directly, instead of having |on_shutdown_event_| relay to |cancel_tasks_flag_|. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:893: DetachFromThread(); How does this work? The comment in weak_ptr.h says: "Calling SupportsWeakPtr::DetachFromThread() can work around the limitations above and cancel the thread binding of the object and all WeakPtrs pointing ot it, but it's not recommended and unsafe." Does this mean DetachFromThread() invalidates |delegate| ? https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:66: // Deletes the delegate on the task runner thread. This is called when the The part that starts with "This is called" is a repeat of the comments in the CancelPendingTasksAndDeleteDelegate() interface declaration. Just say something like "Called by CancelPendingTasksAndDeleteDelegate(). Performs cleanup that needs to happen on |media_task_runner_|. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:86: // complete. Signaled on a different thread. I think the comment is a given. If a thread is waiting, it can't be the one signaling. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/file_syste... File webkit/fileapi/file_system_operation_context.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/file_syste... webkit/fileapi/file_system_operation_context.h:44: mtp_device_delegate_ = delegate->GetAsWeakPtrOnIOThread(); This is no longer a simple setter if you call GetAsWeakPtrOnIOThread(). There's only one set_mtp_device_delegate() caller, so can we call GetAsWeakPtrOnIOThread() there instead? https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_file_util.cc (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_file_util.cc:67: return base::PLATFORM_FILE_ERROR_NOT_FOUND; Perhaps PLATFORM_FILE_ERROR_FAILED is more appropriate? Ditto below. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_delegate.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:61: // pending requests before posting any message to delete itself. Called on the Can you put the "Called..." part on the next line? https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:65: // Called by FileSystemOperationContext to get MTPDeviceDelegate as a WeakPtr. Again, you don't get to dictate who calls you. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_map_service.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.h:20: // This class provides media transfer protocol(MTP) device delegate to nit: space after protocol https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.h:21: // complete media file system operations. ScopedMTPDeviceMapEntry class I think it's fine to mention related classes, but the comments to the methods don't belong here. If someone changes ScopedMTPDeviceMapEntry, how will they know they have to also update a comment in another file? Comments should explain how the code is used, not exactly who gets to call it from where.
https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:61: // media transfer protocol(MTP) device. This object is constructed for each MTP nit: space after protocol. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:163: typedef std::map<const FilePath::StringType, ExtensionGalleriesHostSet> Isn't this just used as another way of ref-counting a ScopedMTPDeviceMapEntry?
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:61: // media transfer protocol(MTP) device. This object is constructed for each MTP On 2012/11/21 02:32:48, Lei Zhang wrote: > nit: space after protocol. Done. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:163: typedef std::map<const FilePath::StringType, ExtensionGalleriesHostSet> On 2012/11/21 02:32:48, Lei Zhang wrote: > Isn't this just used as another way of ref-counting a ScopedMTPDeviceMapEntry? Yes. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; On 2012/11/21 01:33:30, Lei Zhang wrote: > Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it possible > for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can store > a pointer to CancellationFlag and check that directly, instead of having > |on_shutdown_event_| relay to |cancel_tasks_flag_|. Yes, we need both |cancel_tasks_flag_| and |on_shutdown_event_|. MTPDeviceDelegateImplLinux can be destroyed while the callback is called. There is a chance of dereferencing an invalid pointer in the callback handler. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:893: DetachFromThread(); On 2012/11/21 01:33:30, Lei Zhang wrote: > How does this work? The comment in weak_ptr.h says: > > "Calling SupportsWeakPtr::DetachFromThread() can work around the limitations > above and cancel the thread binding of the object and all WeakPtrs pointing ot > it, but it's not recommended and unsafe." > > Does this mean DetachFromThread() invalidates |delegate| ? DetachFromThread() does not nullify the |delegate| object. It invalidates the thread binding details of the |delegate| object. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:30: // supports weak pointers because fileapi::MTPDeviceDelegate supports weak On 2012/11/20 06:25:33, kinuko wrote: > nit: fileapi::MTPDeviceDelegate -> 'the base class' might make it slightly > easier to follow (for me) Done. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:40: // MTPDeviceDelegate: On 2012/11/20 06:25:33, kinuko wrote: > nit: MTPDeviceDelegate overrides ? During MTPDeviceDelegateWin CL review, pkasting@ mentioned that I don't need to specify the word "overrides" in the comment and he said it is fine to just say "MTPDeviceDelegate:". So, I am following that format. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:66: // Deletes the delegate on the task runner thread. This is called when the On 2012/11/21 01:33:30, Lei Zhang wrote: > The part that starts with "This is called" is a repeat of the comments in the > CancelPendingTasksAndDeleteDelegate() interface declaration. Just say something > like "Called by CancelPendingTasksAndDeleteDelegate(). Performs cleanup that > needs to happen on |media_task_runner_|. Done. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h:86: // complete. Signaled on a different thread. On 2012/11/21 01:33:30, Lei Zhang wrote: > I think the comment is a given. If a thread is waiting, it can't be the one > signaling. Removed the comment. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/file_syste... File webkit/fileapi/file_system_operation_context.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/file_syste... webkit/fileapi/file_system_operation_context.h:44: mtp_device_delegate_ = delegate->GetAsWeakPtrOnIOThread(); On 2012/11/21 01:33:30, Lei Zhang wrote: > This is no longer a simple setter if you call GetAsWeakPtrOnIOThread(). There's > only one set_mtp_device_delegate() caller, so can we call > GetAsWeakPtrOnIOThread() there instead? sure. Done. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_file_util.cc (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_file_util.cc:67: return base::PLATFORM_FILE_ERROR_NOT_FOUND; On 2012/11/21 01:33:30, Lei Zhang wrote: > Perhaps PLATFORM_FILE_ERROR_FAILED is more appropriate? Ditto below. kinuko preferred base::PLATFORM_FILE_ERROR_NOT_FOUND in CreateSnapshotFile. So, I used the same in GetFileInfo(). If the extension requesting the file information has valid file system access permissions, then the only reason |mtp_device_delgate().get()| can be NULL is when the attached MTP device is no longer available to complete the given request. Therefore, base::PLATFORM_FILE_ERROR_NOT_FOUND seems more relevant to the context rather than a generic base::PLATFORM_FILE_ERROR_FAILED message. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_file_util.cc:87: .PassAs<FileSystemFileUtil::AbstractFileEnumerator>(); On 2012/11/20 06:25:33, kinuko wrote: > nit: hmm... if we're just creating a new scoped_ptr (but not passing rvalues) > just using scoped_ptr<foo>(new bar) would look simpler. > > scoped_ptr<FileSystemFileUtil::AbstractFileEnumerator>(new ...) Done. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_delegate.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:61: // pending requests before posting any message to delete itself. Called on the On 2012/11/21 01:33:30, Lei Zhang wrote: > Can you put the "Called..." part on the next line? Done. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:65: // Called by FileSystemOperationContext to get MTPDeviceDelegate as a WeakPtr. On 2012/11/21 01:33:30, Lei Zhang wrote: > Again, you don't get to dictate who calls you. Removed the comment. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_map_service.h (right): https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.h:20: // This class provides media transfer protocol(MTP) device delegate to On 2012/11/21 01:33:30, Lei Zhang wrote: > nit: space after protocol Done. https://codereview.chromium.org/11358243/diff/15013/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.h:21: // complete media file system operations. ScopedMTPDeviceMapEntry class On 2012/11/21 01:33:30, Lei Zhang wrote: > I think it's fine to mention related classes, but the comments to the methods > don't belong here. If someone changes ScopedMTPDeviceMapEntry, how will they > know they have to also update a comment in another file? > > Comments should explain how the code is used, not exactly who gets to call it > from where. Done.
https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:163: typedef std::map<const FilePath::StringType, ExtensionGalleriesHostSet> On 2012/11/21 04:09:53, kmadhusu wrote: > On 2012/11/21 02:32:48, Lei Zhang wrote: > > Isn't this just used as another way of ref-counting a ScopedMTPDeviceMapEntry? > > Yes. So why can't ScopedMTPDeviceMapEntry just be ref-counted? https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; On 2012/11/21 04:09:53, kmadhusu wrote: > On 2012/11/21 01:33:30, Lei Zhang wrote: > > Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it > possible > > for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can > store > > a pointer to CancellationFlag and check that directly, instead of having > > |on_shutdown_event_| relay to |cancel_tasks_flag_|. > > Yes, we need both |cancel_tasks_flag_| and |on_shutdown_event_|. > MTPDeviceDelegateImplLinux can be destroyed while the callback is called. There > is a chance of dereferencing an invalid pointer in the callback handler. Can you explain how we can dereference an invalid pointer? MTPDeviceDelegateImplLinux cannot get destroyed while a worker in running, since it's deleted on MTR, and MTR is waiting in a worker's Run() method. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:893: DetachFromThread(); On 2012/11/21 04:09:53, kmadhusu wrote: > On 2012/11/21 01:33:30, Lei Zhang wrote: > > How does this work? The comment in weak_ptr.h says: > > > > "Calling SupportsWeakPtr::DetachFromThread() can work around the limitations > > above and cancel the thread binding of the object and all WeakPtrs pointing ot > > it, but it's not recommended and unsafe." > > > > Does this mean DetachFromThread() invalidates |delegate| ? > > DetachFromThread() does not nullify the |delegate| object. > It invalidates the thread binding details of the |delegate| object. How about we remove GetAsWeakPtrOnIOThread() altogether? Instead, FileSystemOperationContext::set_mtp_device_delegate() takes a MTPDeviceDelegate*, and FileSystemOperationContext::mtp_device_delegate() returns mtp_device_delegate_->AsWeakPtr(). If you do it this way, WeakPtrs only ever gets created on MTR. https://codereview.chromium.org/11358243/diff/20021/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_map_service.cc (right): https://codereview.chromium.org/11358243/diff/20021/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.cc:62: thread_checker_.DetachFromThread(); Speaking of DetachFromThread(), I don't think this is needed at all.
https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry.h (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry.h:163: typedef std::map<const FilePath::StringType, ExtensionGalleriesHostSet> On 2012/11/21 06:46:39, Lei Zhang wrote: > On 2012/11/21 04:09:53, kmadhusu wrote: > > On 2012/11/21 02:32:48, Lei Zhang wrote: > > > Isn't this just used as another way of ref-counting a > ScopedMTPDeviceMapEntry? > > > > Yes. > > So why can't ScopedMTPDeviceMapEntry just be ref-counted? As we discussed, reverted ScopedMTPDeviceMapEntry to be a RefCounted class. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; On 2012/11/21 06:46:39, Lei Zhang wrote: > On 2012/11/21 04:09:53, kmadhusu wrote: > > On 2012/11/21 01:33:30, Lei Zhang wrote: > > > Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it > > possible > > > for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can > > store > > > a pointer to CancellationFlag and check that directly, instead of having > > > |on_shutdown_event_| relay to |cancel_tasks_flag_|. > > > > Yes, we need both |cancel_tasks_flag_| and |on_shutdown_event_|. > > MTPDeviceDelegateImplLinux can be destroyed while the callback is called. > There > > is a chance of dereferencing an invalid pointer in the callback handler. > > Can you explain how we can dereference an invalid pointer? > MTPDeviceDelegateImplLinux cannot get destroyed while a worker in running, since > it's deleted on MTR, and MTR is waiting in a worker's Run() method. We can signal the MTR from MTPDeviceDelegateImplLinux::CancelPendingTasksAndDeleteDelegate() and cancel all the pending tasks and destruct itself. https://codereview.chromium.org/11358243/diff/15013/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:893: DetachFromThread(); On 2012/11/21 06:46:39, Lei Zhang wrote: > On 2012/11/21 04:09:53, kmadhusu wrote: > > On 2012/11/21 01:33:30, Lei Zhang wrote: > > > How does this work? The comment in weak_ptr.h says: > > > > > > "Calling SupportsWeakPtr::DetachFromThread() can work around the limitations > > > above and cancel the thread binding of the object and all WeakPtrs pointing > ot > > > it, but it's not recommended and unsafe." > > > > > > Does this mean DetachFromThread() invalidates |delegate| ? > > > > DetachFromThread() does not nullify the |delegate| object. > > It invalidates the thread binding details of the |delegate| object. > > How about we remove GetAsWeakPtrOnIOThread() altogether? > > Instead, FileSystemOperationContext::set_mtp_device_delegate() takes a > MTPDeviceDelegate*, and FileSystemOperationContext::mtp_device_delegate() > returns mtp_device_delegate_->AsWeakPtr(). > > If you do it this way, WeakPtrs only ever gets created on MTR. I don't want to store a raw pointer in FileSystemOperationContext. FileSystemOperationContext::mtp_device_delegate() is called on a MTR. By the time, we call mtp_device_delegate() the raw pointer may be invalid. https://codereview.chromium.org/11358243/diff/20021/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_map_service.cc (right): https://codereview.chromium.org/11358243/diff/20021/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_map_service.cc:62: thread_checker_.DetachFromThread(); On 2012/11/21 06:46:39, Lei Zhang wrote: > Speaking of DetachFromThread(), I don't think this is needed at all. When we instantiate thread_checker_, we bind the calling thread to it. I need to call DetachFromThread() from here.
Still thinking about the cancellation flag... https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... chrome/browser/media_gallery/media_file_system_registry.cc:727: device_location, base::Bind( Why not just pass in |mtp_device_delegate_map_| instead of a closure? It makes it much easier to see what's going on when reading the ScopedMTPDeviceMapEntry code by itself. The added flexibility of having a closure isn't needed since we always pass in the same closure. https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:890: // This function is called on the IO thread but the member functions are You can say "The weak pointer is instantiated on the IO thread, but only accessed on |media_task_runner_|. Therefore, detach from the current thread. "This function" is ambiguous, it could mean either GetAsWeakPtrOnIOThread() or AsWeakPtr(). The next sentence has "the the" in it.
https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... chrome/browser/media_gallery/media_file_system_registry.cc:727: device_location, base::Bind( On 2012/11/22 02:25:25, Lei Zhang wrote: > Why not just pass in |mtp_device_delegate_map_| instead of a closure? It makes > it much easier to see what's going on when reading the ScopedMTPDeviceMapEntry > code by itself. The added flexibility of having a closure isn't needed since we > always pass in the same closure. MediaFileSystemRegistry owns |mtp_device_delegate_map_|. |mtp_device_delegate_map_| is an implementation detail of MediaFileSystemRegistry and not ScopedMTPDeviceMapEntry. ScopedMTPDeviceMapEntry should not manage the map which is owned by the consumer (MediaFileSystemRegistry). Consumers of ScopedMTPDeviceMapEntry should be notified about the destruction of the object to do the required clean up. https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:890: // This function is called on the IO thread but the member functions are On 2012/11/22 02:25:25, Lei Zhang wrote: > You can say "The weak pointer is instantiated on the IO thread, but only > accessed on |media_task_runner_|. Therefore, detach from the current thread. > > "This function" is ambiguous, it could mean either GetAsWeakPtrOnIOThread() or > AsWeakPtr(). > > The next sentence has "the the" in it. Done.
https://chromiumcodereview.appspot.com/11358243/diff/15013/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/15013/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; On 2012/11/21 04:09:53, kmadhusu wrote: > On 2012/11/21 01:33:30, Lei Zhang wrote: > > Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it > possible > > for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can > store > > a pointer to CancellationFlag and check that directly, instead of having > > |on_shutdown_event_| relay to |cancel_tasks_flag_|. > > Yes, we need both |cancel_tasks_flag_| and |on_shutdown_event_|. > MTPDeviceDelegateImplLinux can be destroyed while the callback is called. There > is a chance of dereferencing an invalid pointer in the callback handler. So what if we make |cancel_tasks_flag_| a WeakPtr. I think you can make WeakPtrs for CancellationFlags with WeakPtrFactory. https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/20022/chrome/browser/med... chrome/browser/media_gallery/media_file_system_registry.cc:727: device_location, base::Bind( On 2012/11/22 02:41:10, kmadhusu wrote: > On 2012/11/22 02:25:25, Lei Zhang wrote: > > Why not just pass in |mtp_device_delegate_map_| instead of a closure? It makes > > it much easier to see what's going on when reading the ScopedMTPDeviceMapEntry > > code by itself. The added flexibility of having a closure isn't needed since > we > > always pass in the same closure. > > MediaFileSystemRegistry owns |mtp_device_delegate_map_|. > |mtp_device_delegate_map_| is an implementation detail of > MediaFileSystemRegistry and not ScopedMTPDeviceMapEntry. ScopedMTPDeviceMapEntry > should not manage the map which is owned by the consumer > (MediaFileSystemRegistry). Consumers of ScopedMTPDeviceMapEntry should be > notified about the destruction of the object to do the required clean up. Sometimes it's ok for a class to give its private variables out. I think it's less error prone for ScopedMTPDeviceMapEntry to manage its own map entry, but I don't care either way.
https://chromiumcodereview.appspot.com/11358243/diff/15013/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/15013/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:205: base::CancellationFlag cancel_tasks_flag_; On 2012/11/22 03:09:41, Lei Zhang wrote: > On 2012/11/21 04:09:53, kmadhusu wrote: > > On 2012/11/21 01:33:30, Lei Zhang wrote: > > > Do you need both |cancel_tasks_flag_| and |on_shutdown_event_| ? Is it > > possible > > > for MTPDeviceDelegateImplLinux to have a CancellationFlag. The workers can > > store > > > a pointer to CancellationFlag and check that directly, instead of having > > > |on_shutdown_event_| relay to |cancel_tasks_flag_|. > > > > Yes, we need both |cancel_tasks_flag_| and |on_shutdown_event_|. > > MTPDeviceDelegateImplLinux can be destroyed while the callback is called. > There > > is a chance of dereferencing an invalid pointer in the callback handler. > > So what if we make |cancel_tasks_flag_| a WeakPtr. I think you can make WeakPtrs > for CancellationFlags with WeakPtrFactory. WeakPtr's created from a WeakPtrFactory should be dereferenced on a single thread. If we follow your suggested solution, we will create the weak pointer on the media task runner thread and dereference it on a UI thread. We should call DetachFromThread() from CancellationFlag constructor, Cancellation::IsSet() and CancellationFlag::Set() function. Therefore, the suggested solution will not work for this problem. This is just a temporary solution until we fix crbug.com/154835.
https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:178: if (!cancel_tasks_flag_.IsSet()) Can we check is the cancellation flag is set and return early - before |device_handle_| gets set? That way, LazyInit() will fail rather than succeed and go on to attempt to do more work.
https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:178: if (!cancel_tasks_flag_.IsSet()) On 2012/11/27 00:13:01, Lei Zhang wrote: > Can we check is the cancellation flag is set and return early - before > |device_handle_| gets set? That way, LazyInit() will fail rather than succeed > and go on to attempt to do more work. Done.
On 2012/11/27 01:44:03, kmadhusu wrote: > https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... > File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): > > https://chromiumcodereview.appspot.com/11358243/diff/16018/chrome/browser/med... > chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:178: if > (!cancel_tasks_flag_.IsSet()) > On 2012/11/27 00:13:01, Lei Zhang wrote: > > Can we check is the cancellation flag is set and return early - before > > |device_handle_| gets set? That way, LazyInit() will fail rather than succeed > > and go on to attempt to do more work. > > Done. Can we do the same for the other workers?
Modified other worker classes callback handlers to check the cancel_tasks_flag_ status before processing the response data. PTAL. Thanks.
Just return an error for the GetFileInfo worker on shutdown and then lgtm. https://chromiumcodereview.appspot.com/11358243/diff/28014/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/28014/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:292: if (cancel_tasks_flag_.IsSet()) MTPDeviceDelegateImplLinux::GetFileInfo() would end up returning base::PLATFORM_FILE_OK with an empty PlatformFileInfo. ReadFileWorker is also messed up, but that's my fault and I will fix it shortly after this lands.
https://chromiumcodereview.appspot.com/11358243/diff/28014/chrome/browser/med... File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right): https://chromiumcodereview.appspot.com/11358243/diff/28014/chrome/browser/med... chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:292: if (cancel_tasks_flag_.IsSet()) On 2012/11/27 04:10:26, Lei Zhang wrote: > MTPDeviceDelegateImplLinux::GetFileInfo() would end up returning > base::PLATFORM_FILE_OK with an empty PlatformFileInfo. > > ReadFileWorker is also messed up, but that's my fault and I will fix it shortly > after this lands. Good catch. Fixed GetFileInfo worker class.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11358243/16022
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11358243/16022
Retried try job too often for step(s) aura_unit_tests, browser_tests, view_unittests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11358243/16022
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11358243/16022
Message was sent while issue was closed.
Change committed as 169991 |
