Bug 187066 - REGRESSION: [macOS Sierra] TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths is a flaky failure
Summary: REGRESSION: [macOS Sierra] TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-26 15:21 PDT by Dawei Fenton (:realdawei)
Modified: 2018-07-17 16:34 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.97 KB, patch)
2018-07-16 11:34 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2018-07-17 15:46 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (2.89 KB, patch)
2018-07-17 15:54 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dawei Fenton (:realdawei) 2018-06-26 15:21:13 PDT
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
Comment 1 Chris Dumez 2018-06-28 08:28:49 PDT
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.
Comment 2 Radar WebKit Bug Importer 2018-06-28 17:02:12 PDT
<rdar://problem/41609065>
Comment 3 Ryan Haddad 2018-07-02 10:10:55 PDT
This test is frequently making the Sierra bots red. If we aren't going to fix it soon, can we disable it?
Comment 4 Chris Dumez 2018-07-02 10:40:05 PDT
(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.
Comment 5 Ryan Haddad 2018-07-12 14:28:12 PDT
(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?
Comment 6 Sihui Liu 2018-07-16 11:34:10 PDT
Created attachment 345103 [details]
Patch
Comment 7 Brady Eidson 2018-07-17 10:03:43 PDT
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.
Comment 8 Chris Dumez 2018-07-17 10:13:28 PDT
(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.
Comment 9 Chris Dumez 2018-07-17 10:18:45 PDT
(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 10 Chris Dumez 2018-07-17 10:19:59 PDT
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.
Comment 11 Brady Eidson 2018-07-17 10:52:22 PDT
(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)
Comment 12 Brady Eidson 2018-07-17 10:55:15 PDT
(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.
Comment 13 Sihui Liu 2018-07-17 13:15:34 PDT
(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).
Comment 14 Chris Dumez 2018-07-17 13:20:32 PDT
(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.
Comment 15 Brady Eidson 2018-07-17 13:58:50 PDT
(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.
Comment 16 Sihui Liu 2018-07-17 15:46:33 PDT
Created attachment 345199 [details]
Patch
Comment 17 Chris Dumez 2018-07-17 15:48:07 PDT
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
Comment 18 Sihui Liu 2018-07-17 15:54:09 PDT
Created attachment 345201 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2018-07-17 16:34:43 PDT
Comment on attachment 345201 [details]
Patch for landing

Clearing flags on attachment: 345201

Committed r233899: <https://trac.webkit.org/changeset/233899>
Comment 20 WebKit Commit Bot 2018-07-17 16:34:44 PDT
All reviewed patches have been landed.  Closing bug.