Summary: | [ Mac WK2 iOS Sim] Layout Test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky failure | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Roberts <sroberts> | ||||||||||
Component: | Tools / Tests | Assignee: | Sihui Liu <sihui_liu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, commit-queue, dbates, ggaren, lforschler, rniwa, ryanhaddad, sihui_liu, webkit-bot-watchers-bugzilla, webkit-bug-importer, wilander | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=196099 https://bugs.webkit.org/show_bug.cgi?id=199468 |
||||||||||||
Bug Depends on: | 199468 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Shawn Roberts
2019-03-27 11:25:27 PDT
Marked as flaky in https://trac.webkit.org/changeset/243554/webkit while waiting for a fix. Created attachment 369498 [details]
Patch
Created attachment 369499 [details]
Patch
Comment on attachment 369499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369499&action=review > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:157 > + setTimeout(checkWebsiteDataAndContinue, 1000); It would be nice to avoid a fixed timeout, since those are sometimes flaky too. One way to do that is to refactor checkWebsiteDataAndContinue to test for success, and reschedule itself upon failure, up to some limit like 10s. That way, passes can be fast, and only failures are slow. Created attachment 369559 [details]
Patch
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 369499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369499&action=review > > > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:157 > > + setTimeout(checkWebsiteDataAndContinue, 1000); > > It would be nice to avoid a fixed timeout, since those are sometimes flaky > too. > > One way to do that is to refactor checkWebsiteDataAndContinue to test for > success, and reschedule itself upon failure, up to some limit like 10s. That > way, passes can be fast, and only failures are slow. It turns out IDB is not the last data type gets deleted. Even if IDB files are deleted, resource statistics may have not been updated, so we cannot use missing IDB database to decide whether the deletion of website data finishes. Comment on attachment 369559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369559&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:618 > + m_dumpResourceLoadStatisticsFunction = WTFMove(completionHandler); Let's call this m_dumpResourceLoadStatisticsCompletionHandler, and assert that it's null before this sets it. Created attachment 370256 [details]
Patch for landing
Comment on attachment 370256 [details] Patch for landing Clearing flags on attachment: 370256 Committed r245517: <https://trac.webkit.org/changeset/245517> All reviewed patches have been landed. Closing bug. Comment on attachment 370256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370256&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 > + ASSERT(!RunLoop::isMain()); This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue. Comment on attachment 370256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370256&action=review >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 >> + ASSERT(!RunLoop::isMain()); > > This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. > WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue. I will follow-up. (In reply to Chris Dumez from comment #13) > Comment on attachment 370256 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370256&action=review > > >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:628 > >> + ASSERT(!RunLoop::isMain()); > > > > This is not OK, the whole point of our abstraction is that WebResourceLoadStatisticsStore is always constructed / used / destroyed on the *main* thread. > > WebResourceLoadStatisticsStore::m_statisticsStore however is always constructed / used / destroyed on the background queue. > > I will follow-up. Where is the follow up happening? (In reply to Ryosuke Niwa from comment #14) > (In reply to Chris Dumez from comment #13) > > I will follow-up. > > Where is the follow up happening? https://bugs.webkit.org/show_bug.cgi?id=199468 (In reply to Ryan Haddad from comment #15) > (In reply to Ryosuke Niwa from comment #14) > > (In reply to Chris Dumez from comment #13) > > > I will follow-up. > > > > Where is the follow up happening? > https://bugs.webkit.org/show_bug.cgi?id=199468 Thanks! |