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
Beth Dakin
2020-06-10 22:23:58 PDT
Created attachment 401620 [details]
Patch
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. (But it's possible someone Databasey can say that they're OK) 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.
(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. Created attachment 401657 [details]
Patch that leaves SQL in a functioning state
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. (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 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! Created attachment 401667 [details]
Patch
Created attachment 401670 [details]
Patch
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 Committed r262922: <https://trac.webkit.org/changeset/262922> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401670 [details]. |