Deflake ZipFiles/FilesAppBrowserTest.Test/zipFileOpenDownloads_GuestMode

The issue with this test is a bit hard to explain, so I'll start by
giving some background on FileSystemContext/FileSystemOperationRunner,
then on FileSystemFileURLLoader, and then on their racy interaction that
causes the test to be flaky.

StoragePartitionImpl contains a scoped_refptr to a FileSystemContext,
which contains helpers and information about the file system. One of
these helpers is a unique_ptr to a FileSystemOperationRunner, which as
the name implies, manages running operations on the file system (create,
delete, write, etc..).  When you ask the FileSystemOperationRunner to
perform an operation, you give it a callback and whatever else it needs,
and then it starts the operation and keeps some metadata about it keyed
by an OperationId. This metadata contains a scoped_refptr to the
FileSystemContext that owns the FileSystemOperationRunner.  This
reference ensures that while the operation is running, the
FileSystemContext won't be deleted, which in turn keeps the runner alive
until the operation is complete. When the underlying operation is done,
the FileSystemOperationRunner will call the callback, and then clean up
the metadata about the operation, removing the reference to the
FileSystemContext in the process. This could potentially delete the
FileSystemContext, but only after any pending work is complete.

Meanwhile, when the network service is enabled, it installs a
FileSystemFileURLLoader to handle requests to local files. This class's
constructor takes a scoped_refptr to a FileSystemContext, which it uses
to access the file system. When the FileSystemFileURLLoader serves a
file, it will call GetMetadata on the FileSystemOperationRunner
associated with the FileSystemContext it has a reference to, with its
DidGetMetadata method as the callback. DidGetMetadata checks the error
code and does something unimportant (for this CL) on success; however,
on error it will tell the client the request is complete, delete the
client ptr, and then delete itself.

The last thing to know is that when a StoragePartitionImpl is being
deleted, it calls FileSystemContext::Shutdown, which calls
FileSystemOperationRunner::Shutdown, which clears the metadata map that
contained the scoped_refptrs keeping the FileSystemContext alive during
pending operations.

In this test, the following sequence of events can happen:
1. Something requests a resource from disk, resulting in a
   FileSystemFileURLLoader being created, which starts a GetMetadata
   call on the StoragePartition's FileSystemOperationRunner.
   FileSystemOperationRunner will start the operation and add it to its
   map of pending operations.
2. The StoragePartition gets deleted, which calls
   FileSystemOperationRunner::Shutdown, which clears the pending
   operations map. Both the map and StoragePartitions' scoped_refptr to
   the FileSystemContext are now deleted. The only thing keeping the
   FileSystemContext alive is the FileSystemFileURLLoader.
3. The underlying GetMetadata operation completes (was aborted) and
   calls FileSystemOperationRunner::DidGetMetadata. This will call the
   completion callback, FileSystemFileURLLoader::DidGetMetadata, which
   will complete the request, and then delete itself, taking the last
   FileSystemContext reference with it. After the callback is called,
   FileSystemOperationRunner calls FinishOperation to clean up the
   GetMetadata operation, but the callback already deleted the runner,
   so FinishOperation segfaults.

My initial thought was that callbacks shouldn't ever delete the object
they were passed to, but I felt that this issue was subtle enough (to me
at least) that it could come again even if we fixed this case. Instead,
this CL makes the Did* completion callback methods in
FileSystemOperationRunner take a reference to the FileSystemContext to
make sure it won't be deleted until after it's done handling the
operation.

Bug: 930807
Change-Id: Ie35348697c4145aad73cbada3da604f878c1cf00
Reviewed-on: https://chromium-review.googlesource.com/c/1464647
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#631011}
1 file changed