Bug 155169

Summary: synthesizePrototype() and friends need to be followed by exception checks (or equivalent).
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, keith_miller, msaboff, oliver, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 154865    
Attachments:
Description Flags
x86_64 benchmark result.
none
proposed patch.
mark.lam: review-
proposed patch 2: with rebased binding test result. ggaren: review+

Mark Lam
Reported 2016-03-08 07:20:55 PST
Failure to do so can result in the new exceptions being thrown (and thereby obscuring) an existing exception that has been thrown but not handled yet. It may also mean that the VM will continue running on potentially unstable state, and may have undesirable consequences. I first observed this in some failed assertion while running tests on a patch for https://bugs.webkit.org/show_bug.cgi?id=154865.
Attachments
x86_64 benchmark result. (68.60 KB, text/plain)
2016-03-08 07:24 PST, Mark Lam 		no flags
Details
proposed patch. (44.50 KB, patch)
2016-03-08 09:21 PST, Mark Lam 		mark.lam: review-
DetailsFormatted DiffDiff
proposed patch 2: with rebased binding test result. (45.46 KB, patch)
2016-03-08 12:41 PST, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2016-03-08 07:24:04 PST
Created attachment 273294 [details] x86_64 benchmark result. I did a patch where I changed synthesizePrototype() and JSValue::isObjectSlow() to return a nullptr when they fail and an exception is thrown. With that patch, the perf numbers are neutral. Patch coming soon.
Mark Lam
Comment 2 2016-03-08 09:21:38 PST
Created attachment 273299 [details] proposed patch.
Mark Lam
Comment 3 2016-03-08 09:57:22 PST
Comment on attachment 273299 [details] proposed patch. Will rebase bindings test result.
Mark Lam
Comment 4 2016-03-08 12:41:14 PST
Created attachment 273318 [details] proposed patch 2: with rebased binding test result. This patch has been run on the api tests, binding tests, JSC tests, and layout tests with no regression (AFAIK).
Geoffrey Garen
Comment 5 2016-03-08 12:52:56 PST
Comment on attachment 273318 [details] proposed patch 2: with rebased binding test result. r=me
Mark Lam
Comment 6 2016-03-08 12:58:17 PST
Thanks for the review. Landed in r197794: <http://trac.webkit.org/r197794>.
Note You need to log in before you can comment on or make changes to this bug.