WebAssembly: Unexpected "RangeError: Maximum call stack size exceeded."

[Fumiya Chiba]

Tested Version       : Release 42 (Safari 11.1, WebKit 13605.1.10) 
URLs (if applicable) : http://demo.tombo.io/
Other browsers tested:
	Safari (11.0.1): OK
	Chrome (62.0.3202.75): OK
	Chrome Canary (64.0.3254.0): FAIL with another exception
	Firefox (56.0.2): OK
	Firefox Nightly (58.0a1): OK
	
Step to reproduce:

Push play button on center.

Expected result:

Game launches without exceptions.
cf. http://demo.tombo.io/launch-image/launch-image-320x480.png

Actual result:

Unexpected exception, "RangeError: Maximum call stack size exceeded.", is thrown, but stack is not so deep.


Additional information:
- Only wasm version causes the exception.
- Built with patched emscripten, for mainly Objective-C support.
- Using dynamic link feature of emscripten (loading two .wasm files)
- Seems JS function call from wasm function cause this error (not always)

[Yusuke Suzuki]

It means released Safari in high Sierra works fine, and current STP shows regression, correct?

[Fumiya Chiba]

(In reply to Yusuke Suzuki from comment #1)
> It means released Safari in high Sierra works fine, and current STP shows
> regression, correct?

Yes.

[Yusuke Suzuki]

I'll do bisecting after landing some patches.

[Radar WebKit Bug Importer]

<rdar://problem/35337525>

[Yusuke Suzuki]

(In reply to Fumiya Chiba from comment #2)
> (In reply to Yusuke Suzuki from comment #1)
> > It means released Safari in high Sierra works fine, and current STP shows
> > regression, correct?
> 
> Yes.

I've just tested it in High Sierra Safari 11.0, and it throws the same RangeError.
Could you specify which version is ok?

[Fumiya Chiba]

Very sorry, I was confused.

http://demo.tombo.io/ throws RangeError exception in both Safari 11 and STP.


Also, we've made a workaround to avoid RangeError, which reduces WASM->JS calls. It works well in Safari 11.0.1 but causes tab reload in STP. It's available on http://demo2.tombo.io/

[JF Bastien]

Surprising. I can take a look some time this week if you want to assign me the bug, Yusuke. Of course I'm happy if you tackle it as well :-)

[Yusuke Suzuki]

(In reply to JF Bastien from comment #7)
> Surprising. I can take a look some time this week if you want to assign me
> the bug, Yusuke. Of course I'm happy if you tackle it as well :-)

Oh, sounds cool!

[JF Bastien]

I repo'd on vanilla Safari 11 on MacOS:

[Error] RangeError: Maximum call stack size exceeded.
	wasm function: 770 (application-wasm.js:1:2783281)
	wasm function: 773
	wasm function
	_objc_setProperty_atomic_copy
	(anonymous function) (application-wasm.js:1:2783281)
	wasm function
	wasm function: 880
	wasm function: 4954
	wasm function: 829
	wasm function: 4937
	wasm function: 907
	wasm function: 4937
	wasm function: 906
	wasm function: 13001
	wasm function: 8343
	wasm function: 648
	wasm function: 646
	wasm function: 645
	wasm function
	__objc_load_images
	(anonymous function) (application-wasm.js:1:4084505)
	func (application-wasm.js:1:164048)
	callRuntimeCallbacks (application-wasm.js:1:151325)
	ensureInitRuntime (application-wasm.js:1:151841)
	doRun (application-wasm.js:1:5198396)
	(anonymous function) (application-wasm.js:1:5198682)

With this info it should be easy to use compile the .wasm file offline and see the size of the functions in that stack.

Will do so soon (I'm at a standards meeting, but will fit investigation in downtime).

[JF Bastien]

One caveat: because of dynamic linking there's Foundation.wasm and application-wasm.wasm, and the stack trace doesn't say which is which :-)

I should improve that part of our stack tracing! Shouldn't be much of an impediment since I can just look at both.

[JF Bastien]

Created attachment 328043 [details]
patch

[Saam Barati]

Comment on attachment 328043 [details]
patch

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

r=me

> JSTests/wasm/function-tests/double-instance.js:16
> +// our implementation used to set it to UINT_MAX causing an erroneous stack

nit: UINTPTR_MAX

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:462
> +        Value* actualStackLimitPointer = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), instanceValue(), safeCast<int32_t>(Instance::offsetOfActualStackLimitPointer()));
> +        Value* actualStackLimit = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), actualStackLimitPointer);
> +        m_currentBlock->appendNew<MemoryValue>(m_proc, Store, origin(), actualStackLimit, instanceValue(), safeCast<int32_t>(Instance::offsetOfCachedStackLimit()));

nice

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1319
> +    restoreWebAssemblyGlobalState(RestoreCachedStackLimit::No, m_info.memory, instanceValue(), m_proc, m_currentBlock);

Why No here? The above comment makes me think it should be Yes

> Source/JavaScriptCore/wasm/WasmInstance.h:138
> +    void** m_actualStackLimitPointer { nullptr };

Name suggestion: for me, a name like, "pointerToStackLimit" would be more intuitive. WDYT? Up to you if you want to change it or not, but if you do, I'd recommend changing other various methods with this name as well.

[JF Bastien]

Created attachment 328139 [details]
patch

[WebKit Commit Bot]

Comment on attachment 328139 [details]
patch

Clearing flags on attachment: 328139

Committed r225411: <https://trac.webkit.org/changeset/225411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.