Bug 193292

Summary: Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case properly.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, ews-watchlist, fpizlo, keith_miller, mcatanzaro, msaboff, rmorisset, saam, thorton, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch. ysuzuki: review+

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.