Bug 211089 - [JSC] Add $vm.assertEnabled() to suppress Debug crash expected tests in release+assert build
Summary: [JSC] Add $vm.assertEnabled() to suppress Debug crash expected tests in relea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-27 12:11 PDT by Yusuke Suzuki
Modified: 2020-04-27 14:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.24 KB, patch)
2020-04-27 12:22 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2020-04-27 12:25 PDT, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-04-27 12:11:33 PDT
We have ASSERT which only fires when DOM object is wrongly implemented to catch wrong ones.
We are using ASSERT in JSC side since we don't know all the DOM objects are correctly implemented. If it is not correctly implemented, in Release build, JSC is returning sub-optimal results. But in Debug build, we can catch the wrong ones, which allows us to fix the wrong thing while avoiding catastrophic crashes in release build.

However, the test is assuming that ASSERT is not enabled if it is not Debug build. Release & Debug builds work, but Release+Assert does not meet this assumption.

So, skip this based on `$vm.assertEnabled()`.
Comment 1 Yusuke Suzuki 2020-04-27 12:22:59 PDT
Created attachment 397713 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-27 12:25:48 PDT
Created attachment 397715 [details]
Patch
Comment 3 Keith Miller 2020-04-27 12:33:43 PDT
Comment on attachment 397715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397715&action=review

r=me with suggestion

> JSTests/stress/incorrect-put-could-generate-invalid-ic-involving-dictionary-flatten.js:19
> +if (!$vm.assertEnabled()) {

Why not `if (!$vm.assertEnabled()) quit()`
Comment 4 Yusuke Suzuki 2020-04-27 13:39:15 PDT
Comment on attachment 397715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397715&action=review

>> JSTests/stress/incorrect-put-could-generate-invalid-ic-involving-dictionary-flatten.js:19
>> +if (!$vm.assertEnabled()) {
> 
> Why not `if (!$vm.assertEnabled()) quit()`

Sounds good!
Comment 5 Yusuke Suzuki 2020-04-27 14:14:23 PDT
Committed r260784: <https://trac.webkit.org/changeset/260784>
Comment 6 Radar WebKit Bug Importer 2020-04-27 14:15:17 PDT
<rdar://problem/62468324>