WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
WIP Patch
(24.62 KB, patch)
2017-05-02 16:59 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(36.45 KB, patch)
2017-05-03 10:01 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(36.45 KB, patch)
2017-05-03 10:13 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2017-05-02 13:35:55 PDT
<
rdar://problem/31204417
>
Chris Dumez
Comment 2
2017-05-02 13:40:06 PDT
Created
attachment 308846
[details]
Patch
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
Created
attachment 308918
[details]
Patch
Chris Dumez
Comment 14
2017-05-03 10:13:31 PDT
Created
attachment 308920
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug