Bug 171565 – [WK2] Extend processDidCrash delegate to let the client know the reason for the crash

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

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.

[Chris Dumez]

<rdar://problem/31204417>

[Chris Dumez]

Created attachment 308846 [details]
Patch

[Sam Weinig]

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]

(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]

(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]

(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]

Created attachment 308867 [details]
WIP Patch

[Sam Weinig]

(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]

(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]

(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]

(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]

(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]

Created attachment 308918 [details]
Patch

[Chris Dumez]

Created attachment 308920 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308920 [details]
Patch

Clearing flags on attachment: 308920

Committed r216129: <http://trac.webkit.org/changeset/216129>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.