Fetch and remove file system data via WKWebsiteDataStore

• Component: WebKit API
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 445286 [details]
WIP

[Sihui Liu]

Created attachment 445383 [details]
Patch

[Sihui Liu]

Created attachment 445387 [details]
Patch

[youenn fablet]

Comment on attachment 445387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445387&action=review

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
> +            callbackAggregator->m_websiteData.entries.appendVector(entries);

WTFMove(entries)

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1976
> +                callbackAggregator->m_domains.add(domain);

WTFMove(domain)

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2099
> +            callbackAggregator->m_websiteData.entries.appendVector(entries);

WTFMove

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:169
> +            */

Not sure this is needed to keep a Ref of Connection.
IPC::Connection::send(connectionID...) should do the trick.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:346
> +    auto origins = originsString.split("\n");

I would tend to use encoders/decoders like we use elsewhere.
See for instance decodeCachesNames in CacheStorageEngineCaches.
That way you should be able to directly decode/encode ClientOrigin

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:370
> +            auto originFilePath = FileSystem::pathByAppendingComponent(openingOriginDirectory, "origin");

Will we have to create "origin" path elsewhere?
If so, it might be good to add a routine for that.
Should probably be "origin"_s

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:398
> +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), entries = crossThreadCopy(entries)]() mutable {

crossThreadCopy(WTFMove(entries)) might make it more efficient.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:413
> +            auto originFilePath = FileSystem::pathByAppendingComponent(openingOriginDirectory, "origin");

We are creating again "origin" here.
Also, we have the same for loop here and above, so maybe create a helper function taking a lambda that would be called on every origin if one is found might be useful.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:448
> +            originSet.add(origin);

We could probably WTFMove origin

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:491
> +                deletedDomains.append(domain);

We could use a HashSet and pass it to the completion handler if possible.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:494
> +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), domains = crossThreadCopy(deletedDomains)]() mutable {

crossThreadCopy(WTFMove()) will probably make it more efficient, in case of Vector at least.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:88
> +        auto fileSystemStoragePath = typeStoragePath("FileSystem");

_s.
Maybe it would be better to have an enum of all storage type that we could pass to typeStoragePath.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:105
> +    auto originFilePath = FileSystem::pathByAppendingComponent(m_path, "origin");

"origin"_s probably and would be good to have a single routine.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:124
> +    auto originContent = builder.toString().utf8();

It seems better to do the reading/writing or encoding/decoding in the same file.
I would use persistent encoders instead.

[Sihui Liu]

Comment on attachment 445387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445387&action=review

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
>> +            callbackAggregator->m_websiteData.entries.appendVector(entries);
> 
> WTFMove(entries)

Will change.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1976
>> +                callbackAggregator->m_domains.add(domain);
> 
> WTFMove(domain)

Will change.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2099
>> +            callbackAggregator->m_websiteData.entries.appendVector(entries);
> 
> WTFMove

Will change.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:169
>> +            */
> 
> Not sure this is needed to keep a Ref of Connection.
> IPC::Connection::send(connectionID...) should do the trick.

ah didn't see this function; will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:346
>> +    auto origins = originsString.split("\n");
> 
> I would tend to use encoders/decoders like we use elsewhere.
> See for instance decodeCachesNames in CacheStorageEngineCaches.
> That way you should be able to directly decode/encode ClientOrigin

Sure, will try using Persistence::Encoder and Persistence::Decoder

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:370
>> +            auto originFilePath = FileSystem::pathByAppendingComponent(openingOriginDirectory, "origin");
> 
> Will we have to create "origin" path elsewhere?
> If so, it might be good to add a routine for that.
> Should probably be "origin"_s

Yes, will add it

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:398
>> +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), entries = crossThreadCopy(entries)]() mutable {
> 
> crossThreadCopy(WTFMove(entries)) might make it more efficient.

Okay.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:413
>> +            auto originFilePath = FileSystem::pathByAppendingComponent(openingOriginDirectory, "origin");
> 
> We are creating again "origin" here.
> Also, we have the same for loop here and above, so maybe create a helper function taking a lambda that would be called on every origin if one is found might be useful.

Okay.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:448
>> +            originSet.add(origin);
> 
> We could probably WTFMove origin

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:491
>> +                deletedDomains.append(domain);
> 
> We could use a HashSet and pass it to the completion handler if possible.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:494
>> +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), domains = crossThreadCopy(deletedDomains)]() mutable {
> 
> crossThreadCopy(WTFMove()) will probably make it more efficient, in case of Vector at least.

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:88
>> +        auto fileSystemStoragePath = typeStoragePath("FileSystem");
> 
> _s.
> Maybe it would be better to have an enum of all storage type that we could pass to typeStoragePath.

Sounds good.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:105
>> +    auto originFilePath = FileSystem::pathByAppendingComponent(m_path, "origin");
> 
> "origin"_s probably and would be good to have a single routine.

Okay.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:124
>> +    auto originContent = builder.toString().utf8();
> 
> It seems better to do the reading/writing or encoding/decoding in the same file.
> I would use persistent encoders instead.

Sure.

[Sihui Liu]

Created attachment 445498 [details]
Patch

[Sihui Liu]

Created attachment 445518 [details]
Patch

[youenn fablet]

Comment on attachment 445518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445518&action=review

> Source/WTF/wtf/FileSystem.cpp:552
> +        ASSERT(*childType == FileType::Regular);

Worth using a switch.

> Source/WTF/wtf/FileSystem.cpp:553
> +        auto time = FileSystem::fileModificationTime(childPath);

Should this line be removed?
Can we add API tests for this function?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:339
> +    auto handle = FileSystem::openFile(filePath, FileSystem::FileOpenMode::ReadWrite);

handle -> originFileHandle.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:347
> +    auto readText = FileSystem::readEntireFile(handle);

readText -> originData or something like that.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:355
> +}

I think it would be good to have the encoder part next to the decoder part.
That way, if we change one, it is easy to change the other.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:357
> +void NetworkStorageManager::forEachOriginDirectory(Function<void(const String& directory)> apply)

s/Function/const Function&/

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:386
> +                originTypes.add(origin->clientOrigin, OptionSet<WebsiteDataType> { }).iterator->value.add(*type);

Should we also include an origin->topOrigin entry in originTypes?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:452
> +            return originSet.contains(origin.clientOrigin);

Shouldn't it be return originSet.contains(origin.clientOrigin) || originSet.contains(origin.topOrigin) ?
It is worth adding some tests here.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:485
> +            return domains.contains(domain);

Ditto here, should we also consider origin.topOrigin?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:59
> +    };

It would probably make sense to centralise the String -> StorageType and StorageType -> String routines.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:65
> +            storageIdentifier = "FileSystem";

_s

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:99
> +            FileSystem::deleteAllFilesModifiedSince(fileSystemStoragePath, modifiedSinceTime);

Should deleteAllFilesModifiedSince handle the -WallTime::infinity() directly by calling deleteNonEmptyDirectory as an optimization?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:112
> +void OriginStorageManager::createOriginFileIfNecessary(const WebCore::ClientOrigin& origin)

This routine should be next to the routine that reads the file.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:113
> +{

ASSERT(!RunLoop::isMain());?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:114
> +    auto originFilePath = FileSystem::pathByAppendingComponent(m_path, "origin");

It is worth adding a unique routine to the "origin" file, be it used in OriginStorageManager or NetworkStorageManager.
Probably "origin"_s

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:136
> +    auto children = FileSystem::listDirectory(m_path);

ASSERT(!RunLoop::isMain());?

> Tools/ChangeLog:10
> [phone]  Sihui Liu  <[e-mail]>

Double change log

[Sihui Liu]

Created attachment 445685 [details]
Patch

[Sihui Liu]

Comment on attachment 445518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445518&action=review

>> Source/WTF/wtf/FileSystem.cpp:552
>> +        ASSERT(*childType == FileType::Regular);
> 
> Worth using a switch.

Sure.

>> Source/WTF/wtf/FileSystem.cpp:553
>> +        auto time = FileSystem::fileModificationTime(childPath);
> 
> Should this line be removed?
> Can we add API tests for this function?

ah right; this is redundant. Sure I can add a test for it.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:339
>> +    auto handle = FileSystem::openFile(filePath, FileSystem::FileOpenMode::ReadWrite);
> 
> handle -> originFileHandle.

Okay.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:347
>> +    auto readText = FileSystem::readEntireFile(handle);
> 
> readText -> originData or something like that.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:355
>> +}
> 
> I think it would be good to have the encoder part next to the decoder part.
> That way, if we change one, it is easy to change the other.

Will move.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:357
>> +void NetworkStorageManager::forEachOriginDirectory(Function<void(const String& directory)> apply)
> 
> s/Function/const Function&/

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:386
>> +                originTypes.add(origin->clientOrigin, OptionSet<WebsiteDataType> { }).iterator->value.add(*type);
> 
> Should we also include an origin->topOrigin entry in originTypes?

I guess no, because the data should be viewed as opening origin's data (given that we currently only return one domain per record).
E.g. if user visits page from domain A (domain A does not use storage API), and there's some third-party frame from domain B (domain B uses storage API), I think we would classify the data as domain B's storage.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:452
>> +            return originSet.contains(origin.clientOrigin);
> 
> Shouldn't it be return originSet.contains(origin.clientOrigin) || originSet.contains(origin.topOrigin) ?
> It is worth adding some tests here.

Like I mentioned above, record is created only based on opening origin (clientOrigin); so in deletion we also only consider about opening origin.
E.g. if a third party frame (origin B) on a page (origin A) have FileSystem storage, it's data will be stored in [WebKitStoragePath]/originA/originB/, and the fetch data API will only return 1 record like "origin B, FileSystem".
Will try making a test.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:485
>> +            return domains.contains(domain);
> 
> Ditto here, should we also consider origin.topOrigin?

Explained above.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:59
>> +    };
> 
> It would probably make sense to centralise the String -> StorageType and StorageType -> String routines.

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:65
>> +            storageIdentifier = "FileSystem";
> 
> _s

Will add.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:99
>> +            FileSystem::deleteAllFilesModifiedSince(fileSystemStoragePath, modifiedSinceTime);
> 
> Should deleteAllFilesModifiedSince handle the -WallTime::infinity() directly by calling deleteNonEmptyDirectory as an optimization?

Sounds good!

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:112
>> +void OriginStorageManager::createOriginFileIfNecessary(const WebCore::ClientOrigin& origin)
> 
> This routine should be next to the routine that reads the file.

Will move it.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:113
>> +{
> 
> ASSERT(!RunLoop::isMain());?

Technically all originStorageManager code should run on background queue... will add

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:114
>> +    auto originFilePath = FileSystem::pathByAppendingComponent(m_path, "origin");
> 
> It is worth adding a unique routine to the "origin" file, be it used in OriginStorageManager or NetworkStorageManager.
> Probably "origin"_s

Will move this code to NetworkStorageManager.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:136
>> +    auto children = FileSystem::listDirectory(m_path);
> 
> ASSERT(!RunLoop::isMain());?

Will move to NetworkStorageManager

>> Tools/ChangeLog:10
>> [phone]  Sihui Liu  <[e-mail]>
> 
> Double change log

ah, will remove.

[youenn fablet]

> > Shouldn't it be return originSet.contains(origin.clientOrigin) || originSet.contains(origin.topOrigin) ?
> > It is worth adding some tests here.
> 
> Like I mentioned above, record is created only based on opening origin
> (clientOrigin); so in deletion we also only consider about opening origin.
> E.g. if a third party frame (origin B) on a page (origin A) have FileSystem
> storage, it's data will be stored in [WebKitStoragePath]/originA/originB/,
> and the fetch data API will only return 1 record like "origin B, FileSystem".
> Will try making a test.

When registering a third party service worker (origin A/originB), the fetch data API will mention both origin A and origin B. Ditto for DOMCache.
I haven't looked at other types (IDB, local storage), but it would be worth being consistent across storage types, one way or the other.

[Sihui Liu]

Created attachment 445724 [details]
Patch

[Sihui Liu]

Created attachment 445734 [details]
Patch

[Sihui Liu]

(In reply to youenn fablet from comment #11)
> > > Shouldn't it be return originSet.contains(origin.clientOrigin) || originSet.contains(origin.topOrigin) ?
> > > It is worth adding some tests here.
> > 
> > Like I mentioned above, record is created only based on opening origin
> > (clientOrigin); so in deletion we also only consider about opening origin.
> > E.g. if a third party frame (origin B) on a page (origin A) have FileSystem
> > storage, it's data will be stored in [WebKitStoragePath]/originA/originB/,
> > and the fetch data API will only return 1 record like "origin B, FileSystem".
> > Will try making a test.
> 
> When registering a third party service worker (origin A/originB), the fetch
> data API will mention both origin A and origin B. Ditto for DOMCache.
> I haven't looked at other types (IDB, local storage), but it would be worth
> being consistent across storage types, one way or the other.

LocalStorage and IndexedDB does not allow third-party persistent storage.

Generally I think origin in the WebsiteDataRecord should be consistent: either it's top origin in all records, or it's opening origin. Otherwise we will return both origins in the example above, and when clients fetch data after deleting origin A, they will notice origin B is also gone, which is not very intuitive. An better alternative is to return both origins in the same record.

But since we have been using this policy in CacheStorage and ServiceWorker, I will make this consistent.

[Sihui Liu]

Created attachment 445813 [details]
Patch

[youenn fablet]

Comment on attachment 445813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445813&action=review

> Source/WTF/wtf/FileSystem.cpp:517
> +std::optional<Vector<uint8_t>> readEntireFile(PlatformFileHandle handle)

I can see a few places in our code base that could make use of it (createForFile, Engine::readSizeFile).
Let's file a bug to make use of this routine across the code base.

> Source/WebCore/Modules/filesystemaccess/FileSystemStorageConnection.h:30
> +#include "ProcessQualified.h"

Is that needed? Should it be in a cpp file instead?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:49
> +    auto originFileHandle = FileSystem::openFile(filePath, FileSystem::FileOpenMode::ReadWrite);

Should it be FileSystem::FileOpenMode::Read?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:389
> +void NetworkStorageManager::forEachOriginDirectory(Function<void(const String& directory)>&& apply)

apply can probably be const&?
No need for directory name?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:456
> +            // If origin cannot be retrieved, but we are asked to remove data for all origins, remove it.

If origin cannot be retrieved, we probably lost all this data and will not be able to reuse it in the future.
We should probably add release logging to catch this case.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:104
> +            m_fileSystemStorageManager = nullptr;

Why not m_fileSystemStorageManager = nullptr; unconditionally?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:156
> +    defaultBucket().deleteData(types, modifiedSince);

ASSERT(!RunLoop::isMain());?

[youenn fablet]

> Generally I think origin in the WebsiteDataRecord should be consistent:
> either it's top origin in all records, or it's opening origin. Otherwise we
> will return both origins in the example above, and when clients fetch data
> after deleting origin A, they will notice origin B is also gone, which is
> not very intuitive. An better alternative is to return both origins in the
> same record.

Right, we should decide on precisely what we want to surface.
One thing I would expect is that if a user asks to delete all information stored by example.org, both example.org/XX and XX/example.org be deleted.

[Sihui Liu]

Comment on attachment 445813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445813&action=review

>> Source/WTF/wtf/FileSystem.cpp:517
>> +std::optional<Vector<uint8_t>> readEntireFile(PlatformFileHandle handle)
> 
> I can see a few places in our code base that could make use of it (createForFile, Engine::readSizeFile).
> Let's file a bug to make use of this routine across the code base.

Filed: https://bugs.webkit.org/show_bug.cgi?id=233818

>> Source/WebCore/Modules/filesystemaccess/FileSystemStorageConnection.h:30
>> +#include "ProcessQualified.h"
> 
> Is that needed? Should it be in a cpp file instead?

Yes, it seems to be needed by ScriptExecutionContextIdentifier.h, to avoid partial specialization before initialization error.
I didn't add it to ScriptExecutionContextIdentifier.h as I see the usage in a few places and try not touching those

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:49
>> +    auto originFileHandle = FileSystem::openFile(filePath, FileSystem::FileOpenMode::ReadWrite);
> 
> Should it be FileSystem::FileOpenMode::Read?

Yes.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:389
>> +void NetworkStorageManager::forEachOriginDirectory(Function<void(const String& directory)>&& apply)
> 
> apply can probably be const&?
> No need for directory name?

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:456
>> +            // If origin cannot be retrieved, but we are asked to remove data for all origins, remove it.
> 
> If origin cannot be retrieved, we probably lost all this data and will not be able to reuse it in the future.
> We should probably add release logging to catch this case.

Will add.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:104
>> +            m_fileSystemStorageManager = nullptr;
> 
> Why not m_fileSystemStorageManager = nullptr; unconditionally?

Will remove.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:156
>> +    defaultBucket().deleteData(types, modifiedSince);
> 
> ASSERT(!RunLoop::isMain());?

Sure.

[Sihui Liu]

Created attachment 445880 [details]
Patch for landing

[EWS]

Committed r286507 (244845@main): <https://commits.webkit.org/244845@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445880 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86029185>

[Ryan Haddad]

Reverting this in https://bugs.webkit.org/show_bug.cgi?id=233892 due to layout test flakiness.

[Sihui Liu]

Created attachment 446199 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 446200 [details]
Patch for landing

[Sihui Liu]

Comment on attachment 445880 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=445880&action=review

Re-land this patch with a few changes to solve test flakiness issue.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:461
> +            if (filter(WebCore::ClientOrigin { })) {
> +                FileSystem::deleteAllFilesModifiedSince(directory, modifiedSinceTime);
> +                FileSystem::deleteEmptyDirectory(directory);
> +            }

This does not consider types and modifiedSince parameter. Changed to launch a temp OriginStorageManager for deletion.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:59
>      void clearStorageForTesting(CompletionHandler<void()>&&);

Changed this function to only clear persisted state, because bots can run tests in parallel, and it currently clears storage between tests. 
If we actually clear file system storage in one worker after it finishes a test, the test runs in another worker may fail.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:97
> +        FileSystem::deleteNonEmptyDirectory(m_rootPath);

Removed; it should be "deleteEmptyDirectory", and because NetworkStorageManager handles origin file now, the directory is unlikely empty.

[EWS]

Committed r286601 (244926@trunk): <https://commits.webkit.org/244926@trunk>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446200 [details].