| Summary: | Enable VMTraps checks in RETURN_IF_EXCEPTION. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
| Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2021-04-01 14:45:02 PDT
Created attachment 424964 [details]
proposed patch.
Created attachment 425612 [details]
proposed patch.
Comment on attachment 425612 [details]
proposed patch.
r=me with some comments
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. 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. 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>. |