Bug 223578

Summary: Add logging in IndexedDB to help debug flaky quota tests
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch for landing none

Description Sihui Liu 2021-03-22 09:00:48 PDT
With https://bugs.webkit.org/show_bug.cgi?id=222995, we know that the tests fail because IDB disk usage is not zero (see https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2FIndexedDB%2Fstorage-limit-1.https.html&test=http%2Ftests%2FIndexedDB%2Fstorage-limit.https.html). 

It's unusual because we clean IndexedDB data between tests. So add more logging to decide which database it is.
Comment 1 Sihui Liu 2021-03-22 12:41:03 PDT
Created attachment 423921 [details]
Patch
Comment 2 Chris Dumez 2021-03-22 12:46:49 PDT
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 3 Alexey Proskuryakov 2021-03-22 12:48:47 PDT
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.
Comment 4 Alexey Proskuryakov 2021-03-22 12:49:17 PDT
Sorry, didn't intend to overwrite Chris's r+.
Comment 5 Chris Dumez 2021-03-22 12:51:38 PDT
(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.
Comment 6 Sihui Liu 2021-03-23 10:21:06 PDT
(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.
Comment 7 Sihui Liu 2021-03-23 10:29:04 PDT
(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.
Comment 8 Sihui Liu 2021-03-23 12:09:32 PDT
Created attachment 424046 [details]
Patch
Comment 9 Alexey Proskuryakov 2021-03-26 16:48:38 PDT
Comment on attachment 424046 [details]
Patch

I think that all feedback was addressed?
Comment 10 Radar WebKit Bug Importer 2021-03-29 09:13:38 PDT
<rdar://problem/75956789>
Comment 11 EWS 2021-03-30 11:14:07 PDT
Tools/Scripts/svn-apply failed to apply attachment 424046 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 12 Sihui Liu 2021-03-31 11:34:14 PDT
Created attachment 424792 [details]
Patch for landing
Comment 13 EWS 2021-03-31 12:34:56 PDT
Committed r275301: <https://commits.webkit.org/r275301>

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