Bug 198584 – Allow WebKitTestRunner to terminate network process after it finishes service worker file operations

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

Allow WebKitTestRunner to terminate network process after network process finishes all file operation

[youenn fablet]

Created attachment 371451 [details]
Patch

[Geoffrey Garen]

Should the behavior to store to disk be built into testRunner.terminateNetworkProcess? Under what conditions do we want a test to terminate the network process with potential data loss?

[Sihui Liu]

(In reply to Geoffrey Garen from comment #2)
> Should the behavior to store to disk be built into
> testRunner.terminateNetworkProcess? Under what conditions do we want a test
> to terminate the network process with potential data loss?

When network process crashes, it's possible that writes to disk are not done as they happen on the background threads. I think terminateNetworkProcess just simulates the crash.

But if we want to test the case where network process crashes after writes are done, can we just wait for the writes to complete instead of introducing an internal API to force flush to disk?

[youenn fablet]

(In reply to Sihui Liu from comment #3)
> (In reply to Geoffrey Garen from comment #2)
> > Should the behavior to store to disk be built into
> > testRunner.terminateNetworkProcess? Under what conditions do we want a test
> > to terminate the network process with potential data loss?

I initially tried this approach, but this made the patch a bit more complex than this one.
terminateNetworkProcess is currently synchronous and I did not want to change that so that would require using synchronous IPC for making sure the data is written.

I could see some usecases where we would like to crash the network process while it is trying to write Cache API files and see whether we are able to recover from some kind of data corruption. But that may be more easily covered with API tests.

> When network process crashes, it's possible that writes to disk are not done
> as they happen on the background threads. I think terminateNetworkProcess
> just simulates the crash.
> 
> But if we want to test the case where network process crashes after writes
> are done, can we just wait for the writes to complete instead of introducing
> an internal API to force flush to disk?

Waiting for the writes to finish will require an additional IPC call, right?
I had a tentative patch reusing prepareToSuspend and making it AsyncReply, then terminating NetworkProcess on the reply. Overall, this was a bigger patch.

As a side note, another nicety we could add to terminateNetworkProcess would be promisifying it and resolve the promise when WebProcess sees its network process IPC connection get lost.

[Geoffrey Garen]

Comment on attachment 371451 [details]
Patch

Hmmm... I guess if we need to test network process crashes, that is a case where we want to separate data flush from crash.

I don't think it's possible to wait for the data to be flushed; there's no way to know when to stop waiting. We could add a short delay, but in the past that approach has caused test flakiness on our bots. But maybe you have a suggestion for how to wait?

I'll say r=me for now, but if you have a suggestion, we should consider it.

[Sihui Liu]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 371451 [details]
> Patch
> 
> Hmmm... I guess if we need to test network process crashes, that is a case
> where we want to separate data flush from crash.
> 
> I don't think it's possible to wait for the data to be flushed; there's no
> way to know when to stop waiting. We could add a short delay, but in the
> past that approach has caused test flakiness on our bots. But maybe you have
> a suggestion for how to wait?
> 
> I'll say r=me for now, but if you have a suggestion, we should consider it.

For layout test, it may be tricky (do we want to test process crash in layout test?). For API test, we can make use of different web views like storing data via one view and getting from another; or use WKWebsiteDataStore fetchDataRecordsOfTypes, which fetches data on disk. We may also prepare files on disk to pretend writes are done.

[youenn fablet]

(In reply to Sihui Liu from comment #6)
> (In reply to Geoffrey Garen from comment #5)
> > Comment on attachment 371451 [details]
> > Patch
> > 
> > Hmmm... I guess if we need to test network process crashes, that is a case
> > where we want to separate data flush from crash.
> > 
> > I don't think it's possible to wait for the data to be flushed; there's no
> > way to know when to stop waiting. We could add a short delay, but in the
> > past that approach has caused test flakiness on our bots. But maybe you have
> > a suggestion for how to wait?

The idea is to make it good enough to remove flakiness.
Hopefully, the current patch is sufficient for that purpose.
If not, we can make it more robust, for instance by clearing the in memory representation and reading the data from disk before crashing the network process.

> > I'll say r=me for now, but if you have a suggestion, we should consider it.
> 
> For layout test, it may be tricky (do we want to test process crash in
> layout test?). For API test, we can make use of different web views like
> storing data via one view and getting from another; or use
> WKWebsiteDataStore fetchDataRecordsOfTypes, which fetches data on disk. We
> may also prepare files on disk to pretend writes are done.

We do test network process/service worker process crash with layout tests.

[WebKit Commit Bot]

Comment on attachment 371451 [details]
Patch

Clearing flags on attachment: 371451

Committed r246184: <https://trac.webkit.org/changeset/246184>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51507937>