RESOLVED FIXED 171565
[WK2] Extend processDidCrash delegate to let the client know the reason for the crash
https://bugs.webkit.org/show_bug.cgi?id=171565
Summary [WK2] Extend processDidCrash delegate to let the client know the reason for t...
Chris Dumez
Reported 2017-05-02 13:35:39 PDT
Add SPI to query if a web process was terminated due to resource limits. WebKit currently call the processDidCrash() delegate for both crashes and terminations due to resource limits. With such SPI, the client would be able to distinguish one from the other and treat them differently.
Attachments
Patch (7.63 KB, patch)
2017-05-02 13:40 PDT, Chris Dumez
sam: review-
WIP Patch (24.62 KB, patch)
2017-05-02 16:59 PDT, Chris Dumez
no flags
Patch (36.45 KB, patch)
2017-05-03 10:01 PDT, Chris Dumez
no flags
Patch (36.45 KB, patch)
2017-05-03 10:13 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2017-05-02 13:35:55 PDT
Chris Dumez
Comment 2 2017-05-02 13:40:06 PDT
Sam Weinig
Comment 3 2017-05-02 14:10:30 PDT
Comment on attachment 308846 [details] Patch Seems like this should just be a parameter passed to processDidCrash (or maybe a WKErrorRef?). Also, please add an API test for this (you can at least test the termination reason for a simulated crash). That said, what is the use case for differentiating between different types of crashes?
Chris Dumez
Comment 4 2017-05-02 14:14:04 PDT
(In reply to Sam Weinig from comment #3) > Comment on attachment 308846 [details] > Patch > > Seems like this should just be a parameter passed to processDidCrash (or > maybe a WKErrorRef?). Also, please add an API test for this (you can at > least test the termination reason for a simulated crash). I was hoping I would not need to add API for now as things may evolve in the future. For example, we may want to distinguish terminations due to memory from terminations due to CPU. There may be more reasons for termination in the future. Also, on iOS, we are currently unable to differentiate crashes from JetSams. > > That said, what is the use case for differentiating between different types > of crashes? That is on the associated radar: <rdar://problem/31204417> Basically, specific logging on client side.
Chris Dumez
Comment 5 2017-05-02 14:43:45 PDT
(In reply to Chris Dumez from comment #4) > (In reply to Sam Weinig from comment #3) > > Comment on attachment 308846 [details] > > Patch > > > > Seems like this should just be a parameter passed to processDidCrash (or > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > least test the termination reason for a simulated crash). > > I was hoping I would not need to add API for now as things may evolve in the > future. For example, we may want to distinguish terminations due to memory > from terminations due to CPU. There may be more reasons for termination in > the future. > > Also, on iOS, we are currently unable to differentiate crashes from JetSams. > > > > > That said, what is the use case for differentiating between different types > > of crashes? > > That is on the associated radar: > <rdar://problem/31204417> > > Basically, specific logging on client side. This would also be needed for <rdar://problem/31764764>.
Chris Dumez
Comment 6 2017-05-02 16:02:32 PDT
(In reply to Sam Weinig from comment #3) > Comment on attachment 308846 [details] > Patch > > Seems like this should just be a parameter passed to processDidCrash (or > maybe a WKErrorRef?). Also, please add an API test for this (you can at > least test the termination reason for a simulated crash). > > That said, what is the use case for differentiating between different types > of crashes? Do I understand correctly that I am to create a WKPageNavigationClientV1 that has an extra webProcessDidCrashWithReason delegate? This new delegate would take a WKProcessCrashReason enum as parameter.
Chris Dumez
Comment 7 2017-05-02 16:59:34 PDT
Created attachment 308867 [details] WIP Patch
Sam Weinig
Comment 8 2017-05-02 19:51:45 PDT
(In reply to Chris Dumez from comment #6) > (In reply to Sam Weinig from comment #3) > > Comment on attachment 308846 [details] > > Patch > > > > Seems like this should just be a parameter passed to processDidCrash (or > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > least test the termination reason for a simulated crash). > > > > That said, what is the use case for differentiating between different types > > of crashes? > > Do I understand correctly that I am to create a WKPageNavigationClientV1 > that has an extra webProcessDidCrashWithReason delegate? This new delegate > would take a WKProcessCrashReason enum as parameter. Yeah, that was what I was getting at.
Sam Weinig
Comment 9 2017-05-02 19:52:49 PDT
(In reply to Chris Dumez from comment #4) > (In reply to Sam Weinig from comment #3) > > Comment on attachment 308846 [details] > > Patch > > > > Seems like this should just be a parameter passed to processDidCrash (or > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > least test the termination reason for a simulated crash). > > I was hoping I would not need to add API for now as things may evolve in the > future. For example, we may want to distinguish terminations due to memory > from terminations due to CPU. There may be more reasons for termination in > the future. > > Also, on iOS, we are currently unable to differentiate crashes from JetSams. > I don't follow, you did add API, WKPageGetProcessWasTerminatedDueToResourceLimits.
Chris Dumez
Comment 10 2017-05-02 19:56:22 PDT
(In reply to Sam Weinig from comment #9) > (In reply to Chris Dumez from comment #4) > > (In reply to Sam Weinig from comment #3) > > > Comment on attachment 308846 [details] > > > Patch > > > > > > Seems like this should just be a parameter passed to processDidCrash (or > > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > > least test the termination reason for a simulated crash). > > > > I was hoping I would not need to add API for now as things may evolve in the > > future. For example, we may want to distinguish terminations due to memory > > from terminations due to CPU. There may be more reasons for termination in > > the future. > > > > Also, on iOS, we are currently unable to differentiate crashes from JetSams. > > > > I don't follow, you did add API, > WKPageGetProcessWasTerminatedDueToResourceLimits. It was in private header, so private API. Anyway, my WIP patch should go in the direction you suggested.
Sam Weinig
Comment 11 2017-05-02 20:09:58 PDT
(In reply to Chris Dumez from comment #10) > (In reply to Sam Weinig from comment #9) > > (In reply to Chris Dumez from comment #4) > > > (In reply to Sam Weinig from comment #3) > > > > Comment on attachment 308846 [details] > > > > Patch > > > > > > > > Seems like this should just be a parameter passed to processDidCrash (or > > > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > > > least test the termination reason for a simulated crash). > > > > > > I was hoping I would not need to add API for now as things may evolve in the > > > future. For example, we may want to distinguish terminations due to memory > > > from terminations due to CPU. There may be more reasons for termination in > > > the future. > > > > > > Also, on iOS, we are currently unable to differentiate crashes from JetSams. > > > > > > > I don't follow, you did add API, > > WKPageGetProcessWasTerminatedDueToResourceLimits. > > It was in private header, so private API. Anyway, my WIP patch should go in > the direction you suggested. To be fair, all of the C-based code is SPI, but we do still like for it to be tested. 👍
Chris Dumez
Comment 12 2017-05-02 20:28:46 PDT
(In reply to Sam Weinig from comment #11) > (In reply to Chris Dumez from comment #10) > > (In reply to Sam Weinig from comment #9) > > > (In reply to Chris Dumez from comment #4) > > > > (In reply to Sam Weinig from comment #3) > > > > > Comment on attachment 308846 [details] > > > > > Patch > > > > > > > > > > Seems like this should just be a parameter passed to processDidCrash (or > > > > > maybe a WKErrorRef?). Also, please add an API test for this (you can at > > > > > least test the termination reason for a simulated crash). > > > > > > > > I was hoping I would not need to add API for now as things may evolve in the > > > > future. For example, we may want to distinguish terminations due to memory > > > > from terminations due to CPU. There may be more reasons for termination in > > > > the future. > > > > > > > > Also, on iOS, we are currently unable to differentiate crashes from JetSams. > > > > > > > > > > I don't follow, you did add API, > > > WKPageGetProcessWasTerminatedDueToResourceLimits. > > > > It was in private header, so private API. Anyway, my WIP patch should go in > > the direction you suggested. > > To be fair, all of the C-based code is SPI, but we do still like for it to > be tested. 👍 Understood, I will work on a test tomorrow. In the meantime, please let me know if you have feedback on the API naming I chose.
Chris Dumez
Comment 13 2017-05-03 10:01:36 PDT
Chris Dumez
Comment 14 2017-05-03 10:13:31 PDT
WebKit Commit Bot
Comment 15 2017-05-03 11:54:40 PDT
Comment on attachment 308920 [details] Patch Clearing flags on attachment: 308920 Committed r216129: <http://trac.webkit.org/changeset/216129>
WebKit Commit Bot
Comment 16 2017-05-03 11:54:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.