Bug 213068 – Replace instances of whitelist in WebCore with allowlist

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 401620 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

(But it's possible someone Databasey can say that they're OK)

[Beth Dakin]

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.

[Sihui Liu]

(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.

[Beth Dakin]

Created attachment 401657 [details]
Patch that leaves SQL in a functioning state

[Tim Horton]

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.

[Tim Horton]

(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

[Tim Horton]

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!

[Beth Dakin]

Created attachment 401667 [details]
Patch

[Beth Dakin]

Created attachment 401670 [details]
Patch

[EWS]

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

[EWS]

Committed r262922: <https://trac.webkit.org/changeset/262922>

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

[Radar WebKit Bug Importer]

<rdar://problem/64268181>