Bug 213068

Summary: Replace instances of whitelist in WebCore with allowlist
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebCore Misc.Assignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, beidson, cdumez, changseok, dino, esprehn+autocc, ews-watchlist, glenn, japhet, kangil.han, kondapallykalyan, mmaxfield, pdr, sihui_liu, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213092    
Attachments:
Description Flags
Patch
none
Patch
none
Patch that leaves SQL in a functioning state
thorton: review+, ews-feeder: commit-queue-
Patch
thorton: review+
Patch none

Description Beth Dakin 2020-06-10 22:23:58 PDT
Continue the work started in https://bugs.webkit.org/show_bug.cgi?id=213000 and https://bugs.webkit.org/show_bug.cgi?id=213064 to replace instances of whitelist in WebCore with allowlist.
Comment 1 Beth Dakin 2020-06-10 22:35:00 PDT
Created attachment 401620 [details]
Patch
Comment 2 Tim Horton 2020-06-10 22:38:42 PDT
Comment on attachment 401620 [details]
Patch

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

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:608
> -    executeSQLCommand("CREATE TABLE IF NOT EXISTS CacheWhitelistURLs (url TEXT NOT NULL ON CONFLICT FAIL, cache INTEGER NOT NULL ON CONFLICT FAIL)");
> +    executeSQLCommand("CREATE TABLE IF NOT EXISTS CacheAllowlistURLs (url TEXT NOT NULL ON CONFLICT FAIL, cache INTEGER NOT NULL ON CONFLICT FAIL)");

This seems like a no-go.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:623
> -                      "  DELETE FROM CacheWhitelistURLs WHERE cache = OLD.id;"
> +                      "  DELETE FROM CacheAllowlistURLs WHERE cache = OLD.id;"

This seems like a no-go.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:728
> +            SQLiteStatement statement(m_database, "INSERT INTO CacheAllowlistURLs (url, cache) VALUES (?, ?)");

This seems like a no-go.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1158
> +    SQLiteStatement allowlistStatement(m_database, "SELECT url FROM CacheAllowlistURLs WHERE cache=?");

This seems like a no-go.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1173
> +    SQLiteStatement allowlistWildcardStatement(m_database, "SELECT wildcard FROM CacheAllowsAllNetworkRequests WHERE cache=?");

This seems like a no-go.
Comment 3 Tim Horton 2020-06-10 22:39:16 PDT
(But it's possible someone Databasey can say that they're OK)
Comment 4 Beth Dakin 2020-06-11 09:40:11 PDT
Created attachment 401656 [details]
Patch

Here's a patch to try to make the Windows bots happy. I'll ping Brady and Sihui about the database changes.
Comment 5 Sihui Liu 2020-06-11 10:08:29 PDT
(In reply to Beth Dakin from comment #4)
> Created attachment 401656 [details]
> Patch
> 
> Here's a patch to try to make the Windows bots happy. I'll ping Brady and
> Sihui about the database changes.

I think Tim's right. We need to migrate the data from the old table to new table. The current patch creates a new table with new name and leave the old table behind.

For the migration part, you probably can refer to something like
static bool createOrMigrateRecordsTableIfNecessary(SQLiteDatabase& database) in SQLiteIDBBackingStore.cpp: if old table exists, create a temp table and copy the data to temp table, then drop the old table and rename the temp table to new name.
Comment 6 Beth Dakin 2020-06-11 10:09:59 PDT
Created attachment 401657 [details]
Patch that leaves SQL in a functioning state
Comment 7 Tim Horton 2020-06-11 10:51:01 PDT
Comment on attachment 401657 [details]
Patch that leaves SQL in a functioning state

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

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1158
> +    SQLiteStatement allowlistStatement(m_database, "SELECT url FROM CacheWhitelistURLs WHERE cache=?");

You missed one.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1173
> +    SQLiteStatement allowlistWildcardStatement(m_database, "SELECT wildcard FROM CacheWhiteAllNetworkRequests WHERE cache=?");

You missed one.
Comment 8 Tim Horton 2020-06-11 10:51:23 PDT
(In reply to Tim Horton from comment #7)
> Comment on attachment 401657 [details]
> Patch that leaves SQL in a functioning state
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401657&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1158
> > +    SQLiteStatement allowlistStatement(m_database, "SELECT url FROM CacheWhitelistURLs WHERE cache=?");
> 
> You missed one.
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1173
> > +    SQLiteStatement allowlistWildcardStatement(m_database, "SELECT wildcard FROM CacheWhiteAllNetworkRequests WHERE cache=?");
> 
> You missed one.

WAIT NEVERMIND
Comment 9 Tim Horton 2020-06-11 12:11:30 PDT
Comment on attachment 401657 [details]
Patch that leaves SQL in a functioning state

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

>> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1173
>> +    SQLiteStatement allowlistWildcardStatement(m_database, "SELECT wildcard FROM CacheWhiteAllNetworkRequests WHERE cache=?");
> 
> You missed one.

Oh wait, you did actually miss this one!
Comment 10 Beth Dakin 2020-06-11 12:24:42 PDT
Created attachment 401667 [details]
Patch
Comment 11 Beth Dakin 2020-06-11 12:32:47 PDT
Created attachment 401670 [details]
Patch
Comment 12 EWS 2020-06-11 12:43:57 PDT
Found 2 new test failures: http/tests/appcache/crash-when-navigating-away-then-back.html, imported/w3c/web-platform-tests/service-workers/service-worker/claim-fetch-with-appcache.https.html
Comment 13 EWS 2020-06-11 13:47:26 PDT
Committed r262922: <https://trac.webkit.org/changeset/262922>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401670 [details].
Comment 14 Radar WebKit Bug Importer 2020-06-11 13:48:19 PDT
<rdar://problem/64268181>