Summary: | Add logging in IndexedDB to help debug flaky quota tests | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | alecflett, beidson, cdumez, ews-watchlist, jsbell, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sihui Liu
2021-03-22 09:00:48 PDT
Created attachment 423921 [details]
Patch
Comment on attachment 423921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423921&action=review > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) Why isn't this using ShouldPrintUsageDetail enum class? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1273 > + auto origin = FileSystem::pathGetFileName(directory); What is this used for? I don't see it used. Comment on attachment 423921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423921&action=review I'll leave the real review for experts, but I couldn't stop myself from adding a couple comments. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) Nit: it would be good to align the terminology ("log" vs. "print detail" vs. "print usage detail"). > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1281 > + WTFLogAlways("SQLiteIDBBackingStore::databasesSizeForDirectory filePath='%s', database='%s', size=%" PRIu64, file.utf8().data(), databaseName.utf8().data(), fileSize); Are there no privacy concerns with logging these? Seems questionable even in regular browsing, and almost certainly not OK in private browsing. Bur perhaps that's how the top level caller decides whether to pass ShouldPrintUsageDetail::Yes. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2533 > + usage += IDBServer::IDBServer::diskUsage(idbRootPath, origin, shouldPrintusageDetail == StorageQuotaManager::ShouldPrintUsageDetail::Yes); shouldPrintusageDetail - "usage" should be title case too. Sorry, didn't intend to overwrite Chris's r+. (In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 423921 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423921&action=review > > I'll leave the real review for experts, but I couldn't stop myself from > adding a couple comments. > > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) > > Nit: it would be good to align the terminology ("log" vs. "print detail" vs. > "print usage detail"). > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1281 > > + WTFLogAlways("SQLiteIDBBackingStore::databasesSizeForDirectory filePath='%s', database='%s', size=%" PRIu64, file.utf8().data(), databaseName.utf8().data(), fileSize); > > Are there no privacy concerns with logging these? Seems questionable even in > regular browsing, and almost certainly not OK in private browsing. Bur > perhaps that's how the top level caller decides whether to pass > ShouldPrintUsageDetail::Yes. Yes, there are privacy concerns but I understood this as a very temporary change to help debug an issue on the bots, not something we will ship. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2533 > > + usage += IDBServer::IDBServer::diskUsage(idbRootPath, origin, shouldPrintusageDetail == StorageQuotaManager::ShouldPrintUsageDetail::Yes); > > shouldPrintusageDetail - "usage" should be title case too. (In reply to Chris Dumez from comment #2) > Comment on attachment 423921 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423921&action=review > > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) > > Why isn't this using ShouldPrintUsageDetail enum class? > Okay, will use the same class ShouldPrintUsageDetail. > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1273 > > + auto origin = FileSystem::pathGetFileName(directory); > > What is this used for? I don't see it used. Will remove. (In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 423921 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423921&action=review > > I'll leave the real review for experts, but I couldn't stop myself from > adding a couple comments. > > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) > > Nit: it would be good to align the terminology ("log" vs. "print detail" vs. > "print usage detail"). > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1281 > > + WTFLogAlways("SQLiteIDBBackingStore::databasesSizeForDirectory filePath='%s', database='%s', size=%" PRIu64, file.utf8().data(), databaseName.utf8().data(), fileSize); > > Are there no privacy concerns with logging these? Seems questionable even in > regular browsing, and almost certainly not OK in private browsing. Bur > perhaps that's how the top level caller decides whether to pass > ShouldPrintUsageDetail::Yes. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2533 > > + usage += IDBServer::IDBServer::diskUsage(idbRootPath, origin, shouldPrintusageDetail == StorageQuotaManager::ShouldPrintUsageDetail::Yes); > > shouldPrintusageDetail - "usage" should be title case too. (In reply to Chris Dumez from comment #5) > (In reply to Alexey Proskuryakov from comment #3) > > Comment on attachment 423921 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=423921&action=review > > > > I'll leave the real review for experts, but I couldn't stop myself from > > adding a couple comments. > > > > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763 > > > +uint64_t IDBServer::diskUsage(const String& rootDirectory, const ClientOrigin& origin, bool shouldPrintDetail) > > > > Nit: it would be good to align the terminology ("log" vs. "print detail" vs. > > "print usage detail"). > > > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1281 > > > + WTFLogAlways("SQLiteIDBBackingStore::databasesSizeForDirectory filePath='%s', database='%s', size=%" PRIu64, file.utf8().data(), databaseName.utf8().data(), fileSize); > > > > Are there no privacy concerns with logging these? Seems questionable even in > > regular browsing, and almost certainly not OK in private browsing. Bur > > perhaps that's how the top level caller decides whether to pass > > ShouldPrintUsageDetail::Yes. > > Yes, there are privacy concerns but I understood this as a very temporary > change to help debug an issue on the bots, not something we will ship. Like Chris said, this should be used for gathering information from bots only. Based on current flaky rate, we should have the information one or two days after the patch is landed. > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2533 > > > + usage += IDBServer::IDBServer::diskUsage(idbRootPath, origin, shouldPrintusageDetail == StorageQuotaManager::ShouldPrintUsageDetail::Yes); > > > > shouldPrintusageDetail - "usage" should be title case too. Created attachment 424046 [details]
Patch
Comment on attachment 424046 [details]
Patch
I think that all feedback was addressed?
Tools/Scripts/svn-apply failed to apply attachment 424046 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Created attachment 424792 [details]
Patch for landing
Committed r275301: <https://commits.webkit.org/r275301> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424792 [details]. |