NativeMediaFileUtil makes invalid use of WeakPtrs |
||||
Issue descriptionNativeMediaFileUtil performs each operation on the TaskRunner supplied in each FileSystemOperationContext, and binds a WeakPtr<NativeMediaFileUtil> to each task. However, the NativeMediaFileUtil instance is owned by the MediaFileSystemBackend, which is torn-down on the browser IO thread, not on the MediaTaskRunner that is used for the operations' TaskRunners. Various NativeMediaFileUtil methods (e.g. CopyOrMoveFileSync) run on the MediaTaskRunner, but make use of the |media_path_filter_| via a raw pointer, which is an object presently owned by the MediaFileSystemBackend, on the IO thread. Ideally the TaskRunner on which to perform operations should be configured with the NativeMediaFileUtil instance at creation time, rather than passed per-operation, so that the implementation can simply assume that all operations run on the same Sequence, and thereby arrange for use of the MediaPathFilter to be safe.
,
Jun 6 2018
Fixed in https://chromium-review.googlesource.com/c/chromium/src/+/1085527.
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca commit d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca Author: Wez <wez@chromium.org> Date: Wed Jul 11 17:33:11 2018 Simplify implementation of FileSystemOperationRunner. - Use a base::AutoReset instance to manage a single integer tracking whether the completion callback for an operation is invoked before we have actually returned to the caller. - Remove the now-unused OperationHandle and BeginOperationScoper. - Replace SupportsWeakPtr with an internal WeakPtrFactory. - Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_| member. - Temporarily disables the recently-added FileSystemURLLoaderFactoryTest tests, which have incorrect threading, causing WeakPtr checks to fire. Bug: 846985 , 860547 Change-Id: Ia059b1b87ef11f3218aeb6c65d7b8dd62be6c393 Reviewed-on: https://chromium-review.googlesource.com/1074427 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Chris Mumford <cmumford@chromium.org> Reviewed-by: Alexander Alekseev <alemate@chromium.org> Cr-Commit-Position: refs/heads/master@{#574220} [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/chrome/browser/chromeos/file_manager/fileapi_util.cc [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/content/test/BUILD.gn [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/extensions/browser/api/file_system/file_system_api.cc [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/storage/browser/fileapi/file_system_operation_impl.cc [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/storage/browser/fileapi/file_system_operation_impl.h [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/storage/browser/fileapi/file_system_operation_runner.cc [modify] https://crrev.com/d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca/storage/browser/fileapi/file_system_operation_runner.h
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/acf1c986e3c9e31fae898d2413fbe054c26b8cf9 commit acf1c986e3c9e31fae898d2413fbe054c26b8cf9 Author: Wez <wez@chromium.org> Date: Fri Aug 17 19:41:55 2018 Revert "Simplify implementation of FileSystemOperationRunner." This reverts commit d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca. Reason for revert: Speculative revert for issue 864351, which started on Canary channel the day after this was landed. The tests in the FileSystemURLLoaderFactoryTest suite remain disabled, because they hit a NetworkService feature check now, under Cast, if re-enabled. Original change's description: > Simplify implementation of FileSystemOperationRunner. > > - Use a base::AutoReset instance to manage a single integer tracking > whether the completion callback for an operation is invoked before > we have actually returned to the caller. > - Remove the now-unused OperationHandle and BeginOperationScoper. > - Replace SupportsWeakPtr with an internal WeakPtrFactory. > - Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_| > member. > - Temporarily disables the recently-added FileSystemURLLoaderFactoryTest > tests, which have incorrect threading, causing WeakPtr checks to > fire. > > Bug: 846985 , 860547 > Change-Id: Ia059b1b87ef11f3218aeb6c65d7b8dd62be6c393 > Reviewed-on: https://chromium-review.googlesource.com/1074427 > Commit-Queue: Wez <wez@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > Reviewed-by: Chris Mumford <cmumford@chromium.org> > Reviewed-by: Alexander Alekseev <alemate@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574220} TBR=wez@chromium.org,rockot@chromium.org,alemate@chromium.org,noel@chromium.org,fukino@chromium.org,cmumford@chromium.org,tzik@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 846985 , 860547, 864351 Change-Id: Ib858c73226785d9fbef3459d49e6cc3fd670d040 Reviewed-on: https://chromium-review.googlesource.com/1179782 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#584152} [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/chrome/browser/chromeos/file_manager/fileapi_util.cc [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/extensions/browser/api/file_system/file_system_api.cc [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/storage/browser/fileapi/file_system_operation_impl.cc [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/storage/browser/fileapi/file_system_operation_impl.h [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/storage/browser/fileapi/file_system_operation_runner.cc [modify] https://crrev.com/acf1c986e3c9e31fae898d2413fbe054c26b8cf9/storage/browser/fileapi/file_system_operation_runner.h
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/42d651f451a5ec58cb4a24d119d13064748445ac commit 42d651f451a5ec58cb4a24d119d13064748445ac Author: Wez <wez@chromium.org> Date: Fri Aug 17 19:50:51 2018 Revert "Simplify implementation of FileSystemOperationRunner." This reverts commit d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca. Reason for revert: Speculative revert for issue 864351, which started on Canary channel the day after this was landed. The tests in the FileSystemURLLoaderFactoryTest suite remain disabled, because they hit a NetworkService feature check now, under Cast, if re-enabled. Original change's description: > Simplify implementation of FileSystemOperationRunner. > > - Use a base::AutoReset instance to manage a single integer tracking > whether the completion callback for an operation is invoked before > we have actually returned to the caller. > - Remove the now-unused OperationHandle and BeginOperationScoper. > - Replace SupportsWeakPtr with an internal WeakPtrFactory. > - Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_| > member. > - Temporarily disables the recently-added FileSystemURLLoaderFactoryTest > tests, which have incorrect threading, causing WeakPtr checks to > fire. > > Bug: 846985 , 860547 > Change-Id: Ia059b1b87ef11f3218aeb6c65d7b8dd62be6c393 > Reviewed-on: https://chromium-review.googlesource.com/1074427 > Commit-Queue: Wez <wez@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > Reviewed-by: Chris Mumford <cmumford@chromium.org> > Reviewed-by: Alexander Alekseev <alemate@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574220} TBR=wez@chromium.org,rockot@chromium.org,alemate@chromium.org,noel@chromium.org,fukino@chromium.org,cmumford@chromium.org,tzik@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 846985 , 860547, 864351 Change-Id: Ib858c73226785d9fbef3459d49e6cc3fd670d040 Reviewed-on: https://chromium-review.googlesource.com/1179782 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Wez <wez@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#584152}(cherry picked from commit acf1c986e3c9e31fae898d2413fbe054c26b8cf9) Reviewed-on: https://chromium-review.googlesource.com/1179344 Cr-Commit-Position: refs/branch-heads/3525@{#7} Cr-Branched-From: f7f1772a44a00254f19f4ddae1f8890ef766e1e3-refs/heads/master@{#583911} [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/chrome/browser/chromeos/file_manager/fileapi_util.cc [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/extensions/browser/api/file_system/file_system_api.cc [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/storage/browser/fileapi/file_system_operation_impl.cc [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/storage/browser/fileapi/file_system_operation_impl.h [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/storage/browser/fileapi/file_system_operation_runner.cc [modify] https://crrev.com/42d651f451a5ec58cb4a24d119d13064748445ac/storage/browser/fileapi/file_system_operation_runner.h
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f8d145c626187d44697d80e69b9040cdadd8f231 commit f8d145c626187d44697d80e69b9040cdadd8f231 Author: Wez <wez@chromium.org> Date: Mon Aug 20 21:34:22 2018 Revert "Simplify implementation of FileSystemOperationRunner." This reverts commit d3c0d24685a29a2c27afc95f0d5e17fb3a0eb2ca. Reason for revert: Speculative revert for issue 864351, which started on Canary channel the day after this was landed. The tests in the FileSystemURLLoaderFactoryTest suite remain disabled, because they hit a NetworkService feature check now, under Cast, if re-enabled. Original change's description: > Simplify implementation of FileSystemOperationRunner. > > - Use a base::AutoReset instance to manage a single integer tracking > whether the completion callback for an operation is invoked before > we have actually returned to the caller. > - Remove the now-unused OperationHandle and BeginOperationScoper. > - Replace SupportsWeakPtr with an internal WeakPtrFactory. > - Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_| > member. > - Temporarily disables the recently-added FileSystemURLLoaderFactoryTest > tests, which have incorrect threading, causing WeakPtr checks to > fire. > > Bug: 846985 , 860547 > Change-Id: Ia059b1b87ef11f3218aeb6c65d7b8dd62be6c393 > Reviewed-on: https://chromium-review.googlesource.com/1074427 > Commit-Queue: Wez <wez@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > Reviewed-by: Chris Mumford <cmumford@chromium.org> > Reviewed-by: Alexander Alekseev <alemate@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574220} TBR=wez@chromium.org,rockot@chromium.org,alemate@chromium.org,noel@chromium.org,fukino@chromium.org,cmumford@chromium.org,tzik@chromium.org Bug: 864351 Change-Id: I4a16bd80faa7e634d001fd16e49c2467f1480c94 Reviewed-on: https://chromium-review.googlesource.com/1182307 Reviewed-by: Wez <wez@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#725} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/chrome/browser/chromeos/file_manager/fileapi_util.cc [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/extensions/browser/api/file_system/file_system_api.cc [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/storage/browser/fileapi/file_system_operation_impl.cc [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/storage/browser/fileapi/file_system_operation_impl.h [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/storage/browser/fileapi/file_system_operation_runner.cc [modify] https://crrev.com/f8d145c626187d44697d80e69b9040cdadd8f231/storage/browser/fileapi/file_system_operation_runner.h
,
Aug 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3639ab9fd75a78e660dd716d265f1f9e69a9716f commit 3639ab9fd75a78e660dd716d265f1f9e69a9716f Author: Wez <wez@chromium.org> Date: Sat Aug 25 00:19:02 2018 Reland "Simplify implementation of FileSystemOperationRunner." This reverts commit acf1c986e3c9e31fae898d2413fbe054c26b8cf9, which speculatively reverted the change to establish whether it might be responsible for a spike in BlobReader crashes. Re-landing now, since the revert had no impact on crash rate of that signature in Canary (see https://crbug.com/864351). - Use a base::AutoReset instance to manage a single integer tracking whether the completion callback for an operation is invoked before we have actually returned to the caller. - Remove the now-unused OperationHandle and BeginOperationScoper. - Replace SupportsWeakPtr with an internal WeakPtrFactory. - Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_| member. - Temporarily disables the recently-added FileSystemURLLoaderFactoryTest tests, which have incorrect threading, causing WeakPtr checks to fire. TBR=wez@chromium.org,rockot@chromium.org,alemate@chromium.org,noel@chromium.org,mek@chromium.org,fukino@chromium.org,cmumford@chromium.org,govind@chromium.org,tzik@chromium.org,abdulsyed@chromium.org Bug: 846985 , 860547, 864351 Change-Id: Ia843811a2e85be9347f410ec9f97dc3dc0f1713a Reviewed-on: https://chromium-review.googlesource.com/1189048 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#586074} [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/chrome/browser/chromeos/file_manager/fileapi_util.cc [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/extensions/browser/api/file_system/file_system_api.cc [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/storage/browser/fileapi/file_system_operation_impl.cc [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/storage/browser/fileapi/file_system_operation_impl.h [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/storage/browser/fileapi/file_system_operation_runner.cc [modify] https://crrev.com/3639ab9fd75a78e660dd716d265f1f9e69a9716f/storage/browser/fileapi/file_system_operation_runner.h |
||||
►
Sign in to add a comment |
||||
Comment 1 by w...@chromium.org
, May 31 2018