WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
198584
Allow WebKitTestRunner to terminate network process after it finishes service worker file operations
https://bugs.webkit.org/show_bug.cgi?id=198584
Summary
Allow WebKitTestRunner to terminate network process after it finishes service...
youenn fablet
Reported
2019-06-05 15:36:26 PDT
Allow WebKitTestRunner to terminate network process after network process finishes all file operation
Attachments
Patch
(12.21 KB, patch)
2019-06-05 16:57 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2019-06-05 16:57:22 PDT
Created
attachment 371451
[details]
Patch
Geoffrey Garen
Comment 2
2019-06-06 11:41:12 PDT
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
Comment 3
2019-06-06 12:08:17 PDT
(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
Comment 4
2019-06-06 15:33:09 PDT
(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 5
2019-06-06 15:35:40 PDT
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
Comment 6
2019-06-06 16:08:40 PDT
(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
Comment 7
2019-06-06 17:59:37 PDT
(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 8
2019-06-06 18:28:59 PDT
Comment on
attachment 371451
[details]
Patch Clearing flags on attachment: 371451 Committed
r246184
: <
https://trac.webkit.org/changeset/246184
>
WebKit Commit Bot
Comment 9
2019-06-06 18:29:01 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10
2019-06-06 18:29:17 PDT
<
rdar://problem/51507937
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug