Bug 242294 – REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[yonihemi]

Created attachment 460636 [details]
Crash log.ips

Regression in iOS 16 beta 2 compared to iOS 15.5:
A long-running loop in Warm crashes the Safari tab on slow_path_wasm_loop_osr [Attached crash log].
Loop originates from SwiftWasm code (https://github.com/yonihemi/iOS16BetaCrashJavaScriptKitExample/blob/271c3a5728e4857c95846274e9bf40e39a28c0ae/Sources/iOS16Crash/main.swift#L16). 


Steps to Reproduce:
Open sample at https://yonihemi.github.io/iOS16BetaCrashJavaScriptKitExample/
Tap "This will crash iOS MobileSafari" button.

Actual Results:
Tab crashes and reloads.
Subsequent taps display "A problem repeatedly occurred".

Expected Results:
Dialog appears, tab doesn't crash.

Build Date & Hardware:
iPhone OS 16.0 beta 2 (20A5303i)
Also reproduces in iOS simulator

Additional Builds and Platforms:
Doesn't occur in iOS 15.5
Doesn't occur in Desktop SafariTP Release 147 (Safari 16.0, WebKit 17614.1.14.10.16)
Doesn't occur in Desktop SafariTP Release 148 (Safari 16.0, WebKit 17614.1.17.1)

[Radar WebKit Bug Importer]

<rdar://problem/96375497>

[Sam Sneddon [:gsnedders]]

rdar://91067361

[Kurt Revis]

I was about to file this! Detailed investigation here:

https://github.com/krevis-figma/webkit-jsc-ios16-regression

The bug only happens on iOS, and only if the WebAssembly module is > 10 MB.

We have previously filed this with Apple as FB10107783 and FB10120166.

[Kurt Revis]

Test case: 
https://krevis-figma.github.io/webkit-jsc-ios16-regression/loop-bad.html

The failure was caused by this WebKit commit:
https://github.com/WebKit/WebKit/commit/e38fc8815d7063e888a1eb3ecb2c7c93ba3fbe84

for bug 234587 “Allow loop tier up to the Air tier”:
https://bugs.webkit.org/show_bug.cgi?id=234587

More notes in the page above.

Attached 3 potential patches:

1-add-regression-test.patch: Adds a regression test that covers this case.

2-add-stress-tests.patch: Adds code to `run-jsc-stress-tests` to cover the fallback-to-B3 code path.

3-fix.patch: A potential fix. Probably not correct style, but it works.

This bug makes figma.com completely unusable in iOS and iPadOS 16. We've had > 100 customer complaints, so far. We would love to see it fixed as soon as possible, certainly before iOS 16 ships.

[Kurt Revis]

Created attachment 460662 [details]
Patch to add regression test for this case

[Kurt Revis]

Created attachment 460663 [details]
Patch to cover the fallback-to-B3 code path in stress tests

[Kurt Revis]

Created attachment 460664 [details]
Potential fix

[Yusuke Suzuki]

@Kurt Revis
Thanks! Can you follow to https://github.com/WebKit/WebKit/wiki/Contributing#contributing-code to upload a patch to GitHub with a specific format? WebKit commit requires commit-message in a specific format, which the template should be generated with .git/prepare-commit-msg hook set up with `git webkit setup` (see https://github.com/WebKit/WebKit/wiki/Contributing 's instructions for setting up repository)
And can you combine a test and fix into one commit?

[Justin Michaud]

Hi Kurt,

This issue was being tracked in our internal bug tracker. This also affects Miniclip.com on iOS.

Thank you for your investigation, it matches what I have seen investigating that bug.

Air and B3 have two different approaches to loop osr. In BBQ Air, every loop is included as a potential entry point, so we only need to compile once. For OMG B3, we wish to compile with only one entry point at a time, so that we do not pessimize loops.

1) Your regression test does not reproduce for me, but I have prepared a test case that does. 

2) Your patch seems to be very similar to the fix that I prepared for the Miniclip bug, so I think we should take it. Thank you for your excellent writeup. I will prepare a PR on GitHub. I am happy to go ahead and land it if you want to get back to work, but you should feel free to make your own PR and we can land that to make sure you are publicly credited. 

3) The final solution here is to remove forceB3 completely. We have the WASM LLint now, so I do not think that this mode is useful. There are some possible performance implications of doing that, so we should land this first.

Thanks again, I will post the PR shortly.

[Kurt Revis]

Pull request: https://github.com/WebKit/WebKit/pull/2105

[Kurt Revis]

Thanks for taking a look so quickly. My PR is up.

(Don't really care about credit, but I already went this far, might as well finish the job.)

Not sure why the regression test didn't work for you, but if you've got one that works, I'm happy to replace mine with yours.

[EWS]

Committed 252167@main (a1935ab9387b): <https://commits.webkit.org/252167@main>

Reviewed commits have been landed. Closing PR #2105 and removing active labels.

[Yusuke Suzuki]

*** Bug 242912 has been marked as a duplicate of this bug. ***

[Kurt Revis]

Verified that this is fixed in iOS 16 beta 4.