Bug 187066 – REGRESSION: [macOS Sierra] TestWebKitAPI.WebKit.WebsiteDataStoreCustomPaths is a flaky failure

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dawei Fenton (:realdawei)]

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

[Chris Dumez]

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.

[Radar WebKit Bug Importer]

<rdar://problem/41609065>

[Ryan Haddad]

This test is frequently making the Sierra bots red. If we aren't going to fix it soon, can we disable it?

[Chris Dumez]

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

[Ryan Haddad]

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

[Sihui Liu]

Created attachment 345103 [details]
Patch

[Brady Eidson]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Brady Eidson]

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

[Brady Eidson]

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

[Sihui Liu]

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

[Chris Dumez]

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

[Brady Eidson]

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

[Sihui Liu]

Created attachment 345199 [details]
Patch

[Chris Dumez]

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

[Sihui Liu]

Created attachment 345201 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345201 [details]
Patch for landing

Clearing flags on attachment: 345201

Committed r233899: <https://trac.webkit.org/changeset/233899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.