Bug 207265 - [iOS] Assert that WebProcessProxy::didBecomeUnresponsive is called if the service worker process takes no process assertion
Summary: [iOS] Assert that WebProcessProxy::didBecomeUnresponsive is called if the ser...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-05 05:47 PST by youenn fablet
Modified: 2020-02-10 09:10 PST (History)
2 users (show)

See Also:

Attachments
Patch (1.91 KB, patch)
2020-02-05 05:57 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2020-02-05 06:07 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-02-05 05:47:52 PST 
WebProcessProxy::didBecomeUnresponsive should only terminate a service worker process if the service worker process takes an activity
Comment 1 youenn fablet 2020-02-05 05:57:07 PST 
Created attachment 389799 [details]
Patch
Comment 2 youenn fablet 2020-02-05 06:07:46 PST 
Created attachment 389801 [details]
Patch
Comment 3 Chris Dumez 2020-02-05 08:06:13 PST 
Comment on attachment 389801 [details]
Patch

Looks like a racy assertion to me. A process could be unresponsive before we drop the assertions and then the responsiveness timer would fire. If you want to protect against this, maybe we should just stop the responsiveness timer when we get the response to PrepareToSuspend IPC.
Comment 4 youenn fablet 2020-02-05 23:30:31 PST 
(In reply to Chris Dumez from comment #3)
> Comment on attachment 389801 [details]
> Patch
> 
> Looks like a racy assertion to me. A process could be unresponsive before we
> drop the assertions and then the responsiveness timer would fire. If you
> want to protect against this, maybe we should just stop the responsiveness
> timer when we get the response to PrepareToSuspend IPC.

My guess was that it might help explaining flakiness issues.
I was thinking we could enable/disable the responsiveness timer when we take assertions. I am not familiar with this responsiveness timer logic, maybe that would conflict with logic enabling/disabling the timer by pages.
Disabling the timer when we know the process is about to suspend seems good too.
Comment 5 Chris Dumez 2020-02-06 08:11:37 PST 
(In reply to youenn fablet from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 389801 [details]
> > Patch
> > 
> > Looks like a racy assertion to me. A process could be unresponsive before we
> > drop the assertions and then the responsiveness timer would fire. If you
> > want to protect against this, maybe we should just stop the responsiveness
> > timer when we get the response to PrepareToSuspend IPC.
> 
> My guess was that it might help explaining flakiness issues.
> I was thinking we could enable/disable the responsiveness timer when we take
> assertions. I am not familiar with this responsiveness timer logic, maybe
> that would conflict with logic enabling/disabling the timer by pages.
> Disabling the timer when we know the process is about to suspend seems good
> too.

We see flakiness on debug bots and the responsiveness timer is disabled in debug.
Comment 6 youenn fablet 2020-02-10 09:10:01 PST 
Closing since timer is no longer enabled on debug bots