Bug 213068

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

<rdar://problem/64268181>