TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths is flaky failure (though frequent) on mac Sierra, I haven't been able to repro locally yet, but I'm looking further into it. Here are results from a recent run: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10169/steps/run-api-tests/logs/stdio Failed TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:187 Value of: [[NSFileManager defaultManager] fileExistsAtPath:fileIDBPath.get().path] Actual: true Expected: false
Looks pretty bad, we ask the data store to remove all the IDB data, and wait for its completion handler. Then the IDB database path still exists.
<rdar://problem/41609065>
This test is frequently making the Sierra bots red. If we aren't going to fix it soon, can we disable it?
(In reply to Ryan Haddad from comment #3) > This test is frequently making the Sierra bots red. If we aren't going to > fix it soon, can we disable it? Brady should comment.
(In reply to Chris Dumez from comment #4) > (In reply to Ryan Haddad from comment #3) > > This test is frequently making the Sierra bots red. If we aren't going to > > fix it soon, can we disable it? > > Brady should comment. Brady, any thoughts here?
Created attachment 345103 [details] Patch
I think we need to know *why* this patch fixes the flakiness. It appears to me that if this patch fixes the flakiness then there's a real bug in WebKit that clients could be running up against.
(In reply to Brady Eidson from comment #7) > I think we need to know *why* this patch fixes the flakiness. > > It appears to me that if this patch fixes the flakiness then there's a real > bug in WebKit that clients could be running up against. If a Child process crashes while there are pending callbacks then those callbacks will be resolved upon crash.
(In reply to Chris Dumez from comment #8) > (In reply to Brady Eidson from comment #7) > > I think we need to know *why* this patch fixes the flakiness. > > > > It appears to me that if this patch fixes the flakiness then there's a real > > bug in WebKit that clients could be running up against. > > If a Child process crashes while there are pending callbacks then those > callbacks will be resolved upon crash. Nah, in this case we're terminating the WebProcess and the pending completion handlers would be for the StorageProcess.
Comment on attachment 345103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345103&action=review > Tools/ChangeLog:9 > + Killing the web process actually led to creating another new process and reloading the page "reloading the page" <- Note that WebKit does not do that. It is up to the client so if this is happening then it means the API test is setting a crash handler and calling reload.
(In reply to Chris Dumez from comment #9) > (In reply to Chris Dumez from comment #8) > > (In reply to Brady Eidson from comment #7) > > > I think we need to know *why* this patch fixes the flakiness. > > > > > > It appears to me that if this patch fixes the flakiness then there's a real > > > bug in WebKit that clients could be running up against. > > > > If a Child process crashes while there are pending callbacks then those > > callbacks will be resolved upon crash. > > Nah, in this case we're terminating the WebProcess and the pending > completion handlers would be for the StorageProcess. Actually, you were right. IDB goes to both Storage and Web processes (because private browsing IDB is in the web processes)
(In reply to Brady Eidson from comment #11) > (In reply to Chris Dumez from comment #9) > > (In reply to Chris Dumez from comment #8) > > > (In reply to Brady Eidson from comment #7) > > > > I think we need to know *why* this patch fixes the flakiness. > > > > > > > > It appears to me that if this patch fixes the flakiness then there's a real > > > > bug in WebKit that clients could be running up against. > > > > > > If a Child process crashes while there are pending callbacks then those > > > callbacks will be resolved upon crash. > > > > Nah, in this case we're terminating the WebProcess and the pending > > completion handlers would be for the StorageProcess. > > Actually, you were right. > > IDB goes to both Storage and Web processes (because private browsing IDB is > in the web processes) Wait. Actually looking at the test, there's no callbacks here, though... are there? What callbacks are you talking about here? The test waits for activity from the web process then makes sure files are in place afterwards. That's all.
(In reply to Brady Eidson from comment #12) > (In reply to Brady Eidson from comment #11) > > (In reply to Chris Dumez from comment #9) > > > (In reply to Chris Dumez from comment #8) > > > > (In reply to Brady Eidson from comment #7) > > > > > I think we need to know *why* this patch fixes the flakiness. > > > > > > > > > > It appears to me that if this patch fixes the flakiness then there's a real > > > > > bug in WebKit that clients could be running up against. > > > > > > > > If a Child process crashes while there are pending callbacks then those > > > > callbacks will be resolved upon crash. > > > > > > Nah, in this case we're terminating the WebProcess and the pending > > > completion handlers would be for the StorageProcess. > > > > Actually, you were right. > > > > IDB goes to both Storage and Web processes (because private browsing IDB is > > in the web processes) > > Wait. Actually looking at the test, there's no callbacks here, though... are > there? > > What callbacks are you talking about here? > > The test waits for activity from the web process then makes sure files are > in place afterwards. That's all. My understanding for this problem is: when reloading the page, the messages from the HTML file would be sent again and the message handler kept setting flag receivedScriptMessage true; so we didn't wait for completion handler to finish to continue. But the assertion is supposed to be reached and evaluated after the completion handler finishes (which means the deletion finishes).
(In reply to Sihui Liu from comment #13) > (In reply to Brady Eidson from comment #12) > > (In reply to Brady Eidson from comment #11) > > > (In reply to Chris Dumez from comment #9) > > > > (In reply to Chris Dumez from comment #8) > > > > > (In reply to Brady Eidson from comment #7) > > > > > > I think we need to know *why* this patch fixes the flakiness. > > > > > > > > > > > > It appears to me that if this patch fixes the flakiness then there's a real > > > > > > bug in WebKit that clients could be running up against. > > > > > > > > > > If a Child process crashes while there are pending callbacks then those > > > > > callbacks will be resolved upon crash. > > > > > > > > Nah, in this case we're terminating the WebProcess and the pending > > > > completion handlers would be for the StorageProcess. > > > > > > Actually, you were right. > > > > > > IDB goes to both Storage and Web processes (because private browsing IDB is > > > in the web processes) > > > > Wait. Actually looking at the test, there's no callbacks here, though... are > > there? > > > > What callbacks are you talking about here? > > > > The test waits for activity from the web process then makes sure files are > > in place afterwards. That's all. > > My understanding for this problem is: when reloading the page, the messages > from the HTML file would be sent again and the message handler kept setting > flag receivedScriptMessage true; so we didn't wait for completion handler to > finish to continue. But the assertion is supposed to be reached and > evaluated after the completion handler finishes (which means the deletion > finishes). Who reloads the page? The API test does not seem to register a crash handler.
(In reply to Chris Dumez from comment #14) > (In reply to Sihui Liu from comment #13) > > (In reply to Brady Eidson from comment #12) > > > (In reply to Brady Eidson from comment #11) > > > > (In reply to Chris Dumez from comment #9) > > > > > (In reply to Chris Dumez from comment #8) > > > > > > (In reply to Brady Eidson from comment #7) > > > > > > > I think we need to know *why* this patch fixes the flakiness. > > > > > > > > > > > > > > It appears to me that if this patch fixes the flakiness then there's a real > > > > > > > bug in WebKit that clients could be running up against. > > > > > > > > > > > > If a Child process crashes while there are pending callbacks then those > > > > > > callbacks will be resolved upon crash. > > > > > > > > > > Nah, in this case we're terminating the WebProcess and the pending > > > > > completion handlers would be for the StorageProcess. > > > > > > > > Actually, you were right. > > > > > > > > IDB goes to both Storage and Web processes (because private browsing IDB is > > > > in the web processes) > > > > > > Wait. Actually looking at the test, there's no callbacks here, though... are > > > there? > > > > > > What callbacks are you talking about here? > > > > > > The test waits for activity from the web process then makes sure files are > > > in place afterwards. That's all. > > > > My understanding for this problem is: when reloading the page, the messages > > from the HTML file would be sent again and the message handler kept setting > > flag receivedScriptMessage true; so we didn't wait for completion handler to > > finish to continue. But the assertion is supposed to be reached and > > evaluated after the completion handler finishes (which means the deletion > > finishes). > > Who reloads the page? The API test does not seem to register a crash handler. Just looked at the backtrace on Sihui's computer. It's definitely built in to WebKit, and is WebKit's default behavior when there is no crash handler. The solution seems to be to install a crash handler. That would prevent WebKit's automatic reloading.
Created attachment 345199 [details] Patch
Comment on attachment 345199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345199&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:61 > + // Overwrite thte default policy which launches a new web process and reload page on crash. typo: thte s/reload/reloads
Created attachment 345201 [details] Patch for landing
Comment on attachment 345201 [details] Patch for landing Clearing flags on attachment: 345201 Committed r233899: <https://trac.webkit.org/changeset/233899>
All reviewed patches have been landed. Closing bug.