Summary: | constructObjectFromPropertyDescriptor() is incorrect with partial descriptors | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | isol2 | ||||||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | ashvayka, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=155404 | ||||||||||||
Attachments: |
|
Description
isol2
2018-04-14 12:41:04 PDT
cinfuzz Created attachment 396533 [details]
Patch
Comment on attachment 396533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396533&action=review > Source/JavaScriptCore/runtime/ObjectConstructor.h:98 > + JSObject* result = constructEmptyObject(globalObject); Huh, did this used to be able to throw? Most callsites don't use a ThrowScope but there's still a good handful of them that do... (In reply to Ross Kirsling from comment #3) > > Source/JavaScriptCore/runtime/ObjectConstructor.h:98 > > + JSObject* result = constructEmptyObject(globalObject); > > Huh, did this used to be able to throw? I did some ChangeLog digging, seems like it never threw, even `JSObject* prototype` overload. Just a precaution I suppose? Comment on attachment 396533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396533&action=review >> Source/JavaScriptCore/runtime/ObjectConstructor.h:98 >> + JSObject* result = constructEmptyObject(globalObject); > > Huh, did this used to be able to throw? Most callsites don't use a ThrowScope but there's still a good handful of them that do... This should not throw. You can add `scope.assertNoException();` Created attachment 396566 [details]
Patch
Bring back ThrowScope with an assert, set reviewer.
Created attachment 397059 [details]
Patch
Rebase patch and fix typo in ChangeLog.
I wonder if it's overkill to scope.assertNoException() both inside the function as well as outside? Created attachment 397061 [details]
Patch
Remove inner scope.assertNoException().
(In reply to Ross Kirsling from comment #8) > I wonder if it's overkill to scope.assertNoException() both inside the > function as well as outside? scope.assertNoException() are more useful outside, and I really like the absence of ThrowScope in constructObjectFromPropertyDescriptor(). (In reply to Alexey Shvayka from comment #10) > (In reply to Ross Kirsling from comment #8) > > I wonder if it's overkill to scope.assertNoException() both inside the > > function as well as outside? > > scope.assertNoException() are more useful outside, and I really like the > absence of ThrowScope in constructObjectFromPropertyDescriptor(). Agreed! Committed r260447: <https://trac.webkit.org/changeset/260447> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397061 [details]. |