Bug 223578 – Add logging in IndexedDB to help debug flaky quota tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 423921 [details]
Patch

[Chris Dumez]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Sorry, didn't intend to overwrite Chris's r+.

[Chris Dumez]

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

[Sihui Liu]

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

[Sihui Liu]

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

[Sihui Liu]

Created attachment 424046 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 424046 [details]
Patch

I think that all feedback was addressed?

[Radar WebKit Bug Importer]

<rdar://problem/75956789>

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 424046 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Sihui Liu]

Created attachment 424792 [details]
Patch for landing

[EWS]

Committed r275301: <https://commits.webkit.org/r275301>

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