[webkit-reviews] review canceled: [Bug 223808] Service Worker scripts use too much memory in the network process : [Attachment 424663] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has canceled Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 223808: Service Worker scripts use too much memory in the network process
https://bugs.webkit.org/show_bug.cgi?id=223808

Attachment 424663: Patch

https://bugs.webkit.org/attachment.cgi?id=424663&action=review




--- Comment #19 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 424663
  --> https://bugs.webkit.org/attachment.cgi?id=424663
Patch

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

>> Source/WebCore/ChangeLog:15
>> +	    and greatly reduce memory usage.
> 
> reduce [ dirty ] memory usage

OK

>> Source/WebCore/ChangeLog:36
>> +	    - WebContent (Dirty): 440MB (cold) / 438MB (warm)
> 
> I wonder why the warm case used to use 1.5X - 2X more memory than the cold
case. Maybe this won't matter once we're done optimizing; but it almost
suggests that we have two copies of the script somewhere in both networking and
WebContent.

Yes, I think this deserves a separate investigation. I could not believe the
857MB at first so I took the measurement twice...

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:53
>> +static const uint64_t schemaVersion = 6;
> 
> Is this schema bump sufficient to delete the old database, or do we need to
do something explicit? (We want to avoid leaving old data behind, including in
the case of a restore from a backup from a previous version of WebKit.)

This is sufficient because the following will get called and remove old
databases:
```
static inline void cleanOldDatabases(const String& databaseDirectory)
{
    for (uint64_t version = 1; version < schemaVersion; ++version)
       
SQLiteFileSystem::deleteDatabaseFile(FileSystem::pathByAppendingComponent(datab
aseDirectory, databaseFilenameFromVersion(version)));
}
```

>> Source/WebCore/workers/service/server/SWScriptStorage.cpp:45
>> +	auto hash = crypto->computeHash();
> 
> Is there anything in this code that will add a salt to the hash? If not, I
think we need to add a salt -- otherwise the hash does not confer much secrecy.
(Specifically, anyone searching for a specific URL can just compute the hash
for themselves and then compare. And perhaps there are other attacks; I'm not
an expert, but I know that salt is best practice and also what we use in the
HTTP cache.)

Ok, I will look into adding a Salt. I did not realized we were concerned about
this sort of attack.

>> Source/WebCore/workers/service/server/SWScriptStorage.cpp:94
>> +	return script.copy();
> 
> Do we expect mmap to fail in any reasonable case? If not, I think we should
just fail the operation instead of falling back. I don't love having fallback
behavior that we can't test.

Ok, so there is definitely one case:
1. size is 0 (I found out because some of our API test use empty files).
FileSystem::mapToFile() definitely fails with an empty file.

This code path is definitely tested by API tests.

Then there is a possible second case I thought of:
2. I am assuming the system has some sort of limits on how many files you can
have mmap'd() at the same time. If so, once we reach that limit, it is nicer to
fall back to basic writing.