New issue
Advanced search Search tips

Issue 860547 link

Starred by 5 users
Status: Assigned
Owner:
cmumford@chromium.org
Cc:
kinuko@chromium.org
nhiroki@chromium.org
w...@chromium.org
tzik@chromium.org
c...@chromium.org
pwnall@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

FileSystemURLLoaderFactoryTest runs various components on the wrong Thread/Sequence

Project Member Reported by w...@chromium.org, Jul 5 
The FileSystemURLLoaderFactoryTest browser tests create a "test" FileSystemContext; those use the calling thread as both their file and I/O TaskRunners (see https://cs.chromium.org/chromium/src/storage/browser/test/test_file_system_context.cc?l=26). This appears to be deliberate, so that the test code can then invoke operations directly from the main thread, rather than having to post tasks to the IO thread.

However, the tests are browser tests, so they have a real BrowserThread::IO; some tasks end up running on the FileSystemContext' |io_task_runner_| (i.e. the main thread) while others end up running on the IO thread.

The tests passed in this state because WeakPtr thread checks are implicitly not applied in the case of no WeakPtrs to an object remaining - now that the FileSystemOperationRunner keeps a WeakPtr to itself, this affordance no longer applies, and the threading issues are visible

Comment 1 by w...@chromium.org, Jul 9

Cc: w...@chromium.org
Owner: cmumford@chromium.org
Status: Assigned (was: Started)
Assigning to cmumford@ as owner of these tests.
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Comment 3 by pwnall@chromium.org, Aug 9

Cc: nhiroki@chromium.org tzik@chromium.org
 Issue 860591  has been merged into this issue.

Comment 4 by pwnall@chromium.org, Aug 9

Cc: pwnall@chromium.org kinuko@chromium.org c...@chromium.org
 Issue 847032  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, 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
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 17

Labels: merge-merged-3525
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
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 20

Labels: merge-merged-3497
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

Comment 8 by cma...@chromium.org, Aug 20

Labels: CommitLog-Audit-Violation M-69 Merge-Without-Approval
An audit of the git commit at "https://chromium.googlesource.com/chromium/src.git/+/f8d145c626187d44697d80e69b9040cdadd8f231" found at least one violation. 
 The commit was created by wez@chromium.org and committed by wez@chromium.org.

Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision f8d145c626187d44697d80e69b9040cdadd8f231 was merged to refs/branch-heads/3497 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!

Comment 9 by w...@chromium.org, Aug 20 

Re #8: Approval was granted on issue 864351. :)

Comment 11 by benmason@chromium.org, Aug 21

Labels: -CommitLog-Audit-Violation -Merge-Without-Approval
Approval was given on https://bugs.chromium.org/p/chromium/issues/detail?id=864351#c44 removing violation labels.
Project Member

Comment 12 by bugdroid1@chromium.org, 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