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.
<rdar://problem/31204417>
Created attachment 308846 [details] Patch
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?
(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.
(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>.
(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.
Created attachment 308867 [details] WIP Patch
(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.
(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.
(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.
(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. 👍
(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.
Created attachment 308918 [details] Patch
Created attachment 308920 [details] Patch
Comment on attachment 308920 [details] Patch Clearing flags on attachment: 308920 Committed r216129: <http://trac.webkit.org/changeset/216129>
All reviewed patches have been landed. Closing bug.