RESOLVED FIXED 224078
Enable VMTraps checks in RETURN_IF_EXCEPTION. 
https://bugs.webkit.org/show_bug.cgi?id=224078
Summary Enable VMTraps checks in RETURN_IF_EXCEPTION.
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.