Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Safari Maximum call stack size exceeded #15981

Closed
kjpou1 opened this issue Aug 2, 2019 · 14 comments
Closed

[wasm] Safari Maximum call stack size exceeded #15981

kjpou1 opened this issue Aug 2, 2019 · 14 comments
Assignees

Comments

@kjpou1
Copy link
Contributor

kjpou1 commented Aug 2, 2019

Here is a demo https://github.com/jfizz/mono-wasm-safari-issue.

  • Download the repo as a zip, unzip, then run python server.py
  • Go to http://localhost:8000/index.html
  • Use the file input to choose doc.docx (included in the repo)
  • Wait a few seconds for the Maximum call stack size exceeded

A few things to note:

  • The demo works in Chrome/Firefox/Edge
  • The demo worked in Safari when ran outside the worker context (main thread). So maybe the issue is related to Safari Web Workers.
  • Oddly, adding a debugger; statement in _mono_wasm_invoke_method resolved the issue (with the devtools open and breakpoints enabled)

Originally posted by @jfizz in #8374 (comment)

@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 2, 2019

@jfizz How did you create the worker.js file?

The actual error seems to be coming from using (WordprocessingDocument wDoc = WordprocessingDocument.Open(memoryStream, true)) { inside the code from WordprocessingDocument. Is there any way to actually see what is failing in there?

@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 2, 2019

Humm, wondering if this is coming from converting the memoryStream byte array to a string within the WordprocessingDocument.

@jfizz
Copy link

jfizz commented Aug 2, 2019

The worker.js file was created by concatenating my custom code (at the top), with mono-config.js, runtime.js, and mono.js which are generated when running mono $WASM_SDK/packager.exe.

The source for the line in question can be found here https://github.com/OfficeDev/Open-XML-SDK/blob/master/src/DocumentFormat.OpenXml/Packaging/WordprocessingDocument.cs#L306.

@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 4, 2019

I have been able to recreate the project here locally from master but not the error. Can you make sure the project is outputting the correct files.

Using these packages:

  <ItemGroup>
    <PackageReference Include="DocumentFormat.OpenXml" Version="2.9.1" />
    <PackageReference Include="OpenXmlPowerTools-NetStandard" Version="4.4.21" />
    <PackageReference Include="System.IO.Packaging" Version="4.5.0" />
  </ItemGroup>

Both release and debug versions seem to be running correctly.

@jfizz
Copy link

jfizz commented Aug 6, 2019

Using the latest SDK did the trick. Thanks for the help!

@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 6, 2019

Great to hear!!

@kjpou1 kjpou1 closed this as completed Aug 6, 2019
@jfizz
Copy link

jfizz commented Aug 6, 2019

Unfortunately, I spoke too soon. The latest sdk fixes the issue for the sample doc.docx provided in the repo, but moving to a slightly larger document (still just one page) leads to more Maximum call stack size exceeded in Safari.

I have uploaded the failing doc as doc2.docx to the repo. Not sure if anything more can be done for Safari. Let me know if I can help.

@kjpou1 kjpou1 reopened this Aug 7, 2019
@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 20, 2019

I ended up opening a bug for this one: https://bugs.webkit.org/show_bug.cgi?id=200918

Hopefully they can provide a little assistance.

@kjpou1 kjpou1 self-assigned this Aug 21, 2019
@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 21, 2019

I also checked this on mobile safari and it works correctly. Added that to the comments in the bug opened.

@Daddoon
Copy link

Daddoon commented Sep 17, 2019

Just to note that there will be surely a regression with Safari 13 / iOS 13, if Apple doesn't fix their Safari memory problems on their release the 2019/09/19.

I tested with:

  • Blazor 3.0.0-preview9.19457.4 (released yesterday, 2019/09/16)
  • iOS 13.1 Beta
  • Blazor sample project

With this configuration, Blazor doesn't start at all.
First error is:

Unhandled Promise Rejection: TypeError: 'arguments', 'callee', and 'caller' cannot be accessed in this context.

Workarounded by adding:

<script>var Module;</script>

before the blazor.webassembly.js script tag.

Then we fall in:

Unhandled Promise Rejection: RangeError: Maximum call stack size exceeded.

Unfortunatly, the only way i have found at the moment is to modify blazor.webassembly.js file, and add somehow a setTimeout mecanism on line 1707.

blazor.webassembly.ios13.fix.zip

Here my current workaround. With this fix the Blazor startup process will loose 2 seconds at boot time.

Except that, i didn't have fallen in new regression about Network call and Tasking at boot time, at least on my iPad it works. I mean, i tested also with a Task.Delay of 5 seconds and without this, and my app was working on both case.

Will test on iPhone 6s tomorrow.

This is post is just informational. In my opinion, this is mainly a Safari defect.

EDIT: Tested on iOS 13 on a iPhone 6s and my fix doesn't work with the 1sec timeout but does work with a 2sec timeout.

@kjpou1
Copy link
Contributor Author

kjpou1 commented Sep 23, 2019

@Daddoon Hello Daddoon

This regression is a little different and involves booting emscripten from a worker. All communications with mono wasm is then handed off to the worker. This is also not using Blazor but using the WebAssembly bindings.

Although, your are right about the other regressions. There has been a PR submitted to rectify the <script>var Module;</script> as you pointed out. dotnet/aspnetcore#13945

@Kukunin
Copy link

Kukunin commented Oct 5, 2019

Have the same issue with ffmpeg.js and Safari 13. It works fine on Safari 12. Oddly, can confirm, that adding debugger; solves the problem. happens only in WASM, the asm.js version works good.

I've built the code with debug info, and managed to get a stack trace:

RangeError: Maximum call stack size exceeded.
<?>.wasm-function[_ff_epzs_motion_search]
<?>.wasm-function[_ff_estimate_p_frame_motion]
<?>.wasm-function[_estimate_motion_thread]
<?>.wasm-function[_avcodec_default_execute]
<?>.wasm-function[_ff_mpv_encode_picture]
<?>.wasm-function[_avcodec_encode_video2]
<?>.wasm-function[_do_encode]
<?>.wasm-function[_avcodec_send_frame]
<?>.wasm-function[_do_video_out]
<?>.wasm-function[_reap_filters]
<?>.wasm-function[emterpret]
<?>.wasm-function[emterpret]
wasm-stub
emterpret
(anonymous function) — magic.js:7586
resume — magic.js:6560
(anonymous function) — magic.js:6615
(anonymous function) — streamer_worker.js:1:2741

The implementation of such function is here: https://github.com/FFmpeg/FFmpeg/blob/7da57875b57012478aed546818b2d52985d13793/libavcodec/motion_est_template.c#L976

Also, on building I receive the following warning:

warning: Output contains some very large functions (9747 lines in _ff_epzs_motion_search), consider building source files with -Os or -Oz)

so it might be related with very big function (because of extensive inlining)

I wonder if this makes us one step closer to a clue to the problem

@Kukunin
Copy link

Kukunin commented Oct 5, 2019

Solved for ffmpeg.js by removing a couple of inline attributes. Now, it reports only

warning: Output contains some very large functions (4946 lines in _ff_epzs_motion_search), consider building source files with -Os or -Oz)

but Safari 13 works fine. This is good evidence that huge functions might cause the problem

jaykrell added a commit to jaykrell/mono that referenced this issue Oct 30, 2019
…bytes per call,

as well as the size of the function that recurses. Fixes iOS13 stack problem.

Like mono#17264 but
this time under ifdef wasm, to avoid regressing non-wasm benchmarks.
And, ideally but not yet, use inline functions and/or macros to share the copied code.

Again the recurse/norecurse split.
Reopened because of the observed objdump of wasm,
far more locals than native code, looks like this will save well
over 100 bytes (assuming all locals are nonvolatile and spilled
to some stack, without overlap).

This fixes iOS13 out of stack problem, on a small repro and on Blazor.

So, let's note some more details about wasm and non-wasm.
In non-wasm, when a local doesn't fit in registers, or isn't needed
for a range of code, but needs to be preserved, it goes to stack.
However, stack locations can be reused, for multiple locals, multiple
types, given traditional compiler lifetime analysis, often obvious
via scopes. CPU can write one type, read another. Or if lifetimes
are disjoint, write one type, then write another. It doesn't take
code to change the type/identity of variables on the stack.

However, in wasm, locals have types. Perhaps part of being safe.
Or to aid enregistration by JIT?
"reinterpret" is an actual instruction.
I gather therefore, that stack packing is much less aggressive in wasm
-- integers and floats will remain distinct.
That is why this sort of change is more effective for wasm than for non-wasm.
Guessing.

See mono#17254 (comment).

Thanks to @kjpou1 for relentlessly pursuing and providing a repro and hand holding through wasm build/test cycle.

Non-wasm (micro?)benchmarks will suffer some.

Other than number of locals, could be large recursive functions give grief.
Could also be the many temporaries implied by stack machine, again, with
a large recursive function. Guessing.

Fixes mono#15981.
Should fix mono#15989.
Should fix mono#16144.
Should fix mono#16172.
Should fix mono#16986.
Should fix chanan/BlazorStrap#226.
Should fix dotnet/aspnetcore#15360.
Two examples tested, not all of the above.
Updated form of mono#16786.

Sort of, but not exactly, alternative to mono#16461.
They are not conflicting in what they do, but solve same problem  -- use less stack.
jaykrell added a commit to jaykrell/mono that referenced this issue Nov 1, 2019
…bytes per call,

as well as the size of the function that recurses. Fixes iOS13 stack problem.

Like mono#17264 but
this time under ifdef wasm, to avoid regressing non-wasm benchmarks.
And, ideally but not yet, use inline functions and/or macros to share the copied code.

Again the recurse/norecurse split.
Reopened because of the observed objdump of wasm,
far more locals than native code, looks like this will save well
over 100 bytes (assuming all locals are nonvolatile and spilled
to some stack, without overlap).

This fixes iOS13 out of stack problem, on a small repro and on Blazor.

So, let's note some more details about wasm and non-wasm.
In non-wasm, when a local doesn't fit in registers, or isn't needed
for a range of code, but needs to be preserved, it goes to stack.
However, stack locations can be reused, for multiple locals, multiple
types, given traditional compiler lifetime analysis, often obvious
via scopes. CPU can write one type, read another. Or if lifetimes
are disjoint, write one type, then write another. It doesn't take
code to change the type/identity of variables on the stack.

However, in wasm, locals have types. Perhaps part of being safe.
Or to aid enregistration by JIT?
"reinterpret" is an actual instruction.
I gather therefore, that stack packing is much less aggressive in wasm
-- integers and floats will remain distinct.
That is why this sort of change is more effective for wasm than for non-wasm.
Guessing.

See mono#17254 (comment).

Thanks to @kjpou1 for relentlessly pursuing and providing a repro and hand holding through wasm build/test cycle.

Non-wasm (micro?)benchmarks will suffer some.

Other than number of locals, could be large recursive functions give grief.
Could also be the many temporaries implied by stack machine, again, with
a large recursive function. Guessing.

Fixes mono#15981.
Should fix mono#15989.
Should fix mono#16144.
Should fix mono#16172.
Should fix mono#16986.
Should fix chanan/BlazorStrap#226.
Should fix dotnet/aspnetcore#15360.
Two examples tested, not all of the above.
Updated form of mono#16786.

Sort of, but not exactly, alternative to mono#16461.
They are not conflicting in what they do, but solve same problem  -- use less stack.
jaykrell added a commit to jaykrell/mono that referenced this issue Nov 2, 2019
…bytes per call,

as well as the size of the function that recurses. Fixes iOS13 stack problem.

Like mono#17264 but
this time under ifdef wasm, to avoid regressing non-wasm benchmarks.
And, ideally but not yet, use inline functions and/or macros to share the copied code.

Again the recurse/norecurse split.
Reopened because of the observed objdump of wasm,
far more locals than native code, looks like this will save well
over 100 bytes (assuming all locals are nonvolatile and spilled
to some stack, without overlap).

This fixes iOS13 out of stack problem, on a small repro and on Blazor.

So, let's note some more details about wasm and non-wasm.
In non-wasm, when a local doesn't fit in registers, or isn't needed
for a range of code, but needs to be preserved, it goes to stack.
However, stack locations can be reused, for multiple locals, multiple
types, given traditional compiler lifetime analysis, often obvious
via scopes. CPU can write one type, read another. Or if lifetimes
are disjoint, write one type, then write another. It doesn't take
code to change the type/identity of variables on the stack.

However, in wasm, locals have types. Perhaps part of being safe.
Or to aid enregistration by JIT?
"reinterpret" is an actual instruction.
I gather therefore, that stack packing is much less aggressive in wasm
-- integers and floats will remain distinct.
That is why this sort of change is more effective for wasm than for non-wasm.
Guessing.

See mono#17254 (comment).

Thanks to @kjpou1 for relentlessly pursuing and providing a repro and hand holding through wasm build/test cycle.

Non-wasm (micro?)benchmarks will suffer some.

Other than number of locals, could be large recursive functions give grief.
Could also be the many temporaries implied by stack machine, again, with
a large recursive function. Guessing.

Fixes mono#15981.
Should fix mono#15989.
Should fix mono#16144.
Should fix mono#16172.
Should fix mono#16986.
Should fix chanan/BlazorStrap#226.
Should fix dotnet/aspnetcore#15360.
Two examples tested, not all of the above.
Updated form of mono#16786.

Sort of, but not exactly, alternative to mono#16461.
They are not conflicting in what they do, but solve same problem  -- use less stack.
jaykrell added a commit to jaykrell/mono that referenced this issue Nov 4, 2019
…bytes per call,

as well as the size of the function that recurses. Fixes iOS13 stack problem.

Like mono#17264 but
this time under ifdef wasm, to avoid regressing non-wasm benchmarks.
And, ideally but not yet, use inline functions and/or macros to share the copied code.

Again the recurse/norecurse split.
Reopened because of the observed objdump of wasm,
far more locals than native code, looks like this will save well
over 100 bytes (assuming all locals are nonvolatile and spilled
to some stack, without overlap).

This fixes iOS13 out of stack problem, on a small repro and on Blazor.

So, let's note some more details about wasm and non-wasm.
In non-wasm, when a local doesn't fit in registers, or isn't needed
for a range of code, but needs to be preserved, it goes to stack.
However, stack locations can be reused, for multiple locals, multiple
types, given traditional compiler lifetime analysis, often obvious
via scopes. CPU can write one type, read another. Or if lifetimes
are disjoint, write one type, then write another. It doesn't take
code to change the type/identity of variables on the stack.

However, in wasm, locals have types. Perhaps part of being safe.
Or to aid enregistration by JIT?
"reinterpret" is an actual instruction.
I gather therefore, that stack packing is much less aggressive in wasm
-- integers and floats will remain distinct.
That is why this sort of change is more effective for wasm than for non-wasm.
Guessing.

See mono#17254 (comment).

Thanks to @kjpou1 for relentlessly pursuing and providing a repro and hand holding through wasm build/test cycle.

Non-wasm (micro?)benchmarks will suffer some.

Other than number of locals, could be large recursive functions give grief.
Could also be the many temporaries implied by stack machine, again, with
a large recursive function. Guessing.

Fixes mono#15981.
Should fix mono#15989.
Should fix mono#16144.
Should fix mono#16172.
Should fix mono#16986.
Should fix chanan/BlazorStrap#226.
Should fix dotnet/aspnetcore#15360.
Two examples tested, not all of the above.
Updated form of mono#16786.

Sort of, but not exactly, alternative to mono#16461.
They are not conflicting in what they do, but solve same problem  -- use less stack.
jaykrell added a commit to jaykrell/mono that referenced this issue Nov 4, 2019
…bytes per call,

as well as the size of the function that recurses. Fixes iOS13 stack problem.

Like mono#17264 but
this time under ifdef wasm, to avoid regressing non-wasm benchmarks.
And, ideally but not yet, use inline functions and/or macros to share the copied code.

Again the recurse/norecurse split.
Reopened because of the observed objdump of wasm,
far more locals than native code, looks like this will save well
over 100 bytes (assuming all locals are nonvolatile and spilled
to some stack, without overlap).

This fixes iOS13 out of stack problem, on a small repro and on Blazor.

So, let's note some more details about wasm and non-wasm.
In non-wasm, when a local doesn't fit in registers, or isn't needed
for a range of code, but needs to be preserved, it goes to stack.
However, stack locations can be reused, for multiple locals, multiple
types, given traditional compiler lifetime analysis, often obvious
via scopes. CPU can write one type, read another. Or if lifetimes
are disjoint, write one type, then write another. It doesn't take
code to change the type/identity of variables on the stack.

However, in wasm, locals have types. Perhaps part of being safe.
Or to aid enregistration by JIT?
"reinterpret" is an actual instruction.
I gather therefore, that stack packing is much less aggressive in wasm
-- integers and floats will remain distinct.
That is why this sort of change is more effective for wasm than for non-wasm.
Guessing.

See mono#17254 (comment).

Thanks to @kjpou1 for relentlessly pursuing and providing a repro and hand holding through wasm build/test cycle.

Non-wasm (micro?)benchmarks will suffer some.

Other than number of locals, could be large recursive functions give grief.
Could also be the many temporaries implied by stack machine, again, with
a large recursive function. Guessing.

Fixes mono#15981.
Should fix mono#15989.
Should fix mono#16144.
Should fix mono#16172.
Should fix mono#16986.
Should fix chanan/BlazorStrap#226.
Should fix dotnet/aspnetcore#15360.
Two examples tested, not all of the above.
Updated form of mono#16786.

Sort of, but not exactly, alternative to mono#16461.
They are not conflicting in what they do, but solve same problem  -- use less stack.
@lewing
Copy link
Member

lewing commented Dec 5, 2019

Should be fixed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment