RESOLVED FIXED 193292
Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case properly. 
https://bugs.webkit.org/show_bug.cgi?id=193292
Summary Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case...
Mark Lam
Reported 2019-01-09 12:25:47 PST
<rdar://problem/46485450>
Attachments
proposed patch. (12.07 KB, patch)
2019-01-09 13:05 PST, Mark Lam 		mark.lam: review-
DetailsFormatted DiffDiff
proposed patch. (12.08 KB, patch)
2019-01-09 14:39 PST, Mark Lam 		ysuzuki: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2019-01-09 13:05:00 PST
Created attachment 358729 [details] proposed patch.
Yusuke Suzuki
Comment 2 2019-01-09 14:16:41 PST
Comment on attachment 358729 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=358729&action=review r=me > Source/bmalloc/bmalloc/Gigacage.cpp:218 > + if (!GIGACAGE_ALLOCATION_CAN_FAIL || wasEnabled()) This code clearly says that the following `function` will be expected to cause the crash. So I think this `function` name should be renamed to `crash` or something like this.
Mark Lam
Comment 3 2019-01-09 14:22:48 PST
Comment on attachment 358729 [details] proposed patch. Oops, my fix is incorrect. We are using addPrimitiveDisableCallback() elsewhere to fire watchpoints if the Gigacage is disabled. Hence, I do need to call the callbacks. Will update the fix and post a mew patch shortly.
Mark Lam
Comment 4 2019-01-09 14:39:21 PST
Created attachment 358741 [details] proposed patch.
Yusuke Suzuki
Comment 5 2019-01-09 14:41:07 PST
Comment on attachment 358741 [details] proposed patch. r=me
Mark Lam
Comment 6 2019-01-09 14:45:43 PST
Thanks for the review. Landed in r239787: <http://trac.webkit.org/r239787>.
Michael Catanzaro
Comment 7 2019-01-14 14:48:30 PST
ensureGigacage() is probably no longer an appropriate name for the function, since it no longer ensures Gigacage is allocated, like it used to when originally written. That semantic issue probably contributed to this bug.
Note You need to log in before you can comment on or make changes to this bug.