Bug 224078

Summary: Enable VMTraps checks in RETURN_IF_EXCEPTION.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bburg, benjamin, calvaris, ews-watchlist, gyuyoung.kim, hi, joepeck, keith_miller, mkwst, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
ews-feeder: commit-queue-
proposed patch. keith_miller: review+

Mark Lam
Reported 2021-04-01 14:45:02 PDT
Doing so has many advantages. Details to follow in the ChangeLog. rdar://75037057
Attachments
proposed patch. (70.75 KB, patch)
2021-04-01 17:17 PDT, Mark Lam 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
proposed patch. (87.49 KB, patch)
2021-04-09 05:03 PDT, Mark Lam 		keith_miller: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2021-04-01 17:17:55 PDT
Created attachment 424964 [details] proposed patch.
Mark Lam
Comment 2 2021-04-09 05:03:38 PDT
Created attachment 425612 [details] proposed patch.
Keith Miller
Comment 3 2021-04-09 11:03:51 PDT
Comment on attachment 425612 [details] proposed patch. r=me with some comments
Keith Miller
Comment 4 2021-04-09 11:29:03 PDT
Comment on attachment 425612 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=425612&action=review > Source/JavaScriptCore/runtime/VMTraps.h:54 > + // NeedDebuggerBreak Should this be lower priority than the NeedTermination check? It seems like it wouldn't be great to present the debugger to a user then terminate as soon as they step into an exception check. > Source/JavaScriptCore/runtime/VMTraps.h:55 > + // - services asynchronous debugger break requests. Nit: it's a bit strange that you're using sentences here but each bullet starts with a lower case letter.
Mark Lam
Comment 5 2021-04-09 11:42:43 PDT
Thanks for the review. (In reply to Keith Miller from comment #4) > Comment on attachment 425612 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425612&action=review > > > Source/JavaScriptCore/runtime/VMTraps.h:54 > > + // NeedDebuggerBreak > > Should this be lower priority than the NeedTermination check? It seems like > it wouldn't be great to present the debugger to a user then terminate as > soon as they step into an exception check. I agree. NeedDebuggerBreak should be lower than all events except for NeedExceptionHandling. I'll move it. > > Source/JavaScriptCore/runtime/VMTraps.h:55 > > + // - services asynchronous debugger break requests. > > Nit: it's a bit strange that you're using sentences here but each bullet > starts with a lower case letter. Will fix.
Mark Lam
Comment 6 2021-04-10 09:15:28 PDT
I've applied the changes as well as fixed some comment typos and added a size impact note in the ChangeLog. Landed in r275797: <http://trac.webkit.org/r275797>.
Note You need to log in before you can comment on or make changes to this bug.