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

Blazor hang with marshal methods #7927

Closed
wants to merge 108 commits into from
Closed

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Mar 28, 2023

NOTE: LLVM IR generator updates are now done in #8140
Context: https://github.com/aspnet/AspNetCore-ManualTests/issues/1934

* main:
  [Xamarin.Android.Build.Tasks] Fix Android Version Code for Release builds (dotnet#7795)
Better string handling in LLVM IR generator
@@ -43,6 +46,10 @@ sealed class InputFiles
[Required]
public string AndroidBinUtilsDirectory { get; set; }

public string AndroidNdkDirectory { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does EnableMarshalMethodTracing require the NDK, and not just the clang build tools we already redistribute?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, it's to find libunwind.a!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only libunwind.a, also a few others from the compiler "private" collection (libc++abi.a, libclang_rt.builtins-<ARCH>-android.a)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracing no longer requires NDK to be present on the end user's machine

@@ -171,6 +171,9 @@ sealed class MarshalMethodName
{ 'L', typeof(_jobjectArray) },
};

const string mm_trace_func_enter_name = "_mm_trace_func_enter";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems kinda odd that most of the members here are camelCase or PascalCase, but then the new members are c_snake_case. Should this not be MMTraceFuncEnterName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a standalone C/C++ function, so I used the C/C++ naming convention.

@@ -2085,6 +2087,8 @@ because xbuild doesn't support framework reference assemblies.
ApplicationSharedLibraries="@(_ApplicationSharedLibrary)"
DebugBuild="$(AndroidIncludeDebugSymbols)"
AndroidBinUtilsDirectory="$(AndroidBinUtilsDirectory)"
EnableMarshalMethodTracing="$(_AndroidEnableMarshalMethodTracing)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when $(_AndroidEnableMarshalMethodTracing)=True and $(_AndroidNdkDirectory) isn't set and/or is the "internal" clang tooling? What's the error message look like? Is that error message acceptable?

(Sure, this is a private property for now, but even we make mistakes…)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now it's ArgumentNullException(nameof (ndk)) in the method where we actually use the NDK. Not sure if we ever want to make this property "public", though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer a problem, NDK not required anymore

return mm_class_names[class_index];
}

static uint64_t get_method_id (uint32_t mono_image_index, uint32_t method_token) noexcept
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why "pack" these values into a uint64_t instead of a std::tuple<uint32_t, uint32_t> or some other struct?

Copy link
Contributor Author

@grendello grendello Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a constant, faster to pass (no pointer, less memory access), can easily be used as a selector when looking for method name/description/etc in an array, and takes up less LLVM IR code (easier to read and understand, not to mention generate):

tail call void @_mm_trace_func_enter (i32 126, i32 0, i32 100664310, i8* getelementptr inbounds ([137 x i8], [137 x i8]* @__str.0, i64 0, i64 0)) #2

tail call void %get_func_ptr (i32 126, i32 0, i32 100664310, i8** nonnull align 8 dereferenceable(8) bitcast (void (%class._JNIEnv*, %class._jclass*, %class._jobject*, %class._jobject*, i32, i64)** @native_cb_onItemClick_126_0_60003f6 to i8**)) #2

if it were a pointer to a struct (or tuple) we'd need to output the struct declaration, variable definition, use a pointer which would be another ugly getelementptr invocation. It would make both calls, and especially the 2nd one, even uglier and less readable.

* main:
  [Mono.Android] Bind API-UpsideDownCake Developer Preview 1 (dotnet#7796)
  Bump to dotnet/installer@d109cba3ff 8.0.100-preview.4.23176.5 (dotnet#7921)
Use standalone libunwind instead of the LLVM's

Instead of using autoconf on *nix and cmake on Windows, provide our
own CMakeLists.txt file which works everywhere we need.
Building application with tracing currently fails because of
__cxa_demangle missing symbols (it requires the built-in clang
libunwind which we cannot use) - we will need our own demangler.
Next week.
* main:
  [tests] Remove `net472` multitargeting (dotnet#7932)
  [monodroid] Fix `ld` build error on Nightly Builds. (dotnet#7925)
  Bump to xamarin/Java.Interop/main@0355acf (dotnet#7931)
  [tests] Use msftconnecttest.com in QuoteInvalidQuoteUrlsShouldWork (dotnet#7919)
  [ci] Don't set demands for megapipeline (dotnet#7929)
  * provide library stubs for liblog and libdl
  * include libc++abi demangler directly in the trace library (its
    sources are part of the NDK)
  * better trace output

The only remaining dependency on NDK is the clang builtins
library (compiler-rt).  Should be able to eliminate that too, with some
tweaks.
At XA build time we find and package the compiler's builtins libraries
for all the ABIs we support. This allows us to link libxamarin-app.so
without requiring NDK to be present on end user's machine.

Additionally, traces have now adjusted addresses and offsets for all
frames, to match bionic behavior.

Finally, the entire trace output is logged in a single message. This
makes everything faster, but also makes sure that the trace won't be
interleaved with unrelated messages in logcat.
* main:
  [readme] Add aka.ms links for d17.5. (dotnet#7935)
Also: introduce tracing modes, 'basic` for just the method enter/leave
messages and 'full' to include native and java stack traces on method
entry.
* main:
  [ci] Remove remaining Classic test jobs. (dotnet#7924)
  Add Nightly Tests for Humanizer
* main:
  [ci] Stop building classic test suites. (dotnet#7938)
It appears that the following might be cause of the "hang":

    04-07 19:39:36.121  4859  4859 I chromium: [INFO:CONSOLE(2)] "Uncaught ReferenceError: Blazor is not defined", source: https://0.0.0.0/ (2)

It is probably caused by this JavaScript fragment from Blazor:

    Blazor.start();
    window.__BlazorStarted = true;

And the Blazor class might not be initialized because of a problem with
delegates using the "old" registration mechanism when marshal methods
are enabled (there are quite a few of them in the test app).

lldb didn't reveal anything interesting, process state appeared to be
fine.
* main:
  [Xamarin.Android] Remove OpenTK, sqlite-xamarin, System.EnterpriseServices. (dotnet#7940)
* main:
  Bump to xamarin/Java.Interop/main@a172402 (dotnet#7944)
// TODO: error handling
trace.append ("\n Java stack trace:");
jobject current_thread = env->CallStaticObjectMethod (java_lang_Thread, java_lang_Thread_currentThread);
auto stack_trace_array = static_cast<jobjectArray>(env->CallNonvirtualObjectMethod (current_thread, java_lang_Thread, java_lang_Thread_getStackTrace));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be CallObjectMethod, as java.lang.Thread.getStackTrace() is a virtual (non-final) method.

The same library will be used by marshal methods tracing (by linking it
into `libxamarin-app.so` when tracing is enabled) or will be used by
`libmonodroid.so` (or p/invoked from managed land) if desired.
* main:
  Bump com.android.tools:r8 from 4.0.52 to 8.0.40 (dotnet#7934)
* main:
  [Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7947)
  Bumping to the correct monodroid commit
  Trying to bump monodroid to run debugger-tests
  Pass timeout to runtime
Eventually to be used, optionally, by libmonodroid
* main:
  Convert `/tools` and `/build-tools` projects from `net472` to `$(DotNetStableTargetFramework)` (dotnet#7943)
The DSO is packaged only if marshal methods tracing is enabled or if
native stack traces are enabled (via the `_UseNativeStackTraces` MSBuild
property)

Added a p/invoke which can log any combination of: native, java, managed
stack traces as well as the state of signal handlers.

Updated p/invoke dispatch tables
* main:
  Bump to xamarin/Java.Interop/main@554d819 (dotnet#7951)
  [Microsoft.Android.Sdk.ILLink] fix crash when TZ changes (dotnet#7956)
  [tests] Port 'Xamarin.Android.JcwGen-Tests.JcwGen-Tests' to .NET (dotnet#7949)
  [Xamarin.Android.Build.Tasks] remove `pdb2mdb` (dotnet#7950)
  [ci] Add some extra params to configure the test templates (dotnet#7955)
* main:
  [Xamarin.Android.Build.Tasks] <GenerateJavaStubs/> should open readonly in some cases (dotnet#8129)
  [Mono.Android] suppress/solve more illink warnings (dotnet#8063)
First two instructions implemented (`store` and `ret`), together with
support for TBAA metadata (https://llvm.org/docs/LangRef.html#tbaa-metadata)

`xamarin_app_init` now fully generated.
* main:
  [Xamarin.Android.Build.Tasks] Allow override of `uncompressedGlob` (dotnet#7965)
* main:
  [One .NET] fix 'dotnet publish' with no arguments (dotnet#8137)
  [build] consider `$NUGET_PACKAGES` for `$(XAPackagesDir)` (dotnet#8136)
  Bump external/xamarin-android-tools from `44885bc` to `3cee10b` (dotnet#8121)
* main:
  [tests] Remove `XASdkDeployTests` (dotnet#8139)
* main:
  Bump to google/bundletool@f17ce94a (dotnet#8135)
  [Xamarin.Android.Build.Tasks] Handle IOException in Aapt2Daemon (dotnet#8130)
  [tests] don't set `/uses-sdk@android:targetSdkVersion=34` by default (dotnet#8138)
* main:
  [Mono.Android] Bind and enumify API-34 (dotnet#8116)
* main:
  $(AndroidPackVersionSuffix)=preview.7; net8 is 34.0.0-preview.7 (dotnet#8149)
* main:
  [Xamarin.Android.Build.Tasks] MarshalMethodsAssemblyRewriter+new file (dotnet#8151)
  Bump to dotnet/installer@d2a244f560 8.0.100-preview.7.23325.5 (dotnet#8142)
* main: (306 commits)
  [templates] Remove redundant "template" from display name. (dotnet#8773)
  Bump to xamarin/Java.Interop/main@a7e09b7 (dotnet#8793)
  [build] Include MIT license in most NuGet packages (dotnet#8787)
  Bump to dotnet/installer@893b762b6e 9.0.100-preview.3.24153.2 (dotnet#8782)
  [docs] update notes about `dotnet-trace` and `dotnet-gcdump` (dotnet#8713)
  [Mono.Android] Fix race condition in AndroidMessageHandler (dotnet#8753)
  [ci] Fix SDL Sources Analysis for PRs from forks (dotnet#8785)
  [ci] Add 1ESPT override to MSBuild test stages (dotnet#8784)
  [ci] Do not use @self annotation for templates (dotnet#8783)
  [ci] Migrate to the 1ES template (dotnet#8747)
  [Mono.Android] fix trimming warnings, part 2 (dotnet#8758)
  [Xamarin.Android.Build.Tasks] set `%(DefineConstantsOnly)` for older API levels (dotnet#8777)
  [tests] fix duplicate sources in `NuGet.config` (dotnet#8772)
  Bump to xamarin/monodroid@e13723e701 (dotnet#8771)
  Bump to xamarin/xamarin-android-tools/main@37d79c9 (dotnet#8752)
  Bump to dotnet/installer@d070660282 9.0.100-preview.3.24126.2 (dotnet#8763)
  Bump to xamarin/java.interop/main@14a9470 (dotnet#8766)
  $(AndroidPackVersionSuffix)=preview.3; net9 is 34.99.0.preview.3 (dotnet#8765)
  [Mono.Android] Do not dispose request content stream in AndroidMessageHandler (dotnet#8764)
  Bump com.android.tools:r8 from 8.2.42 to 8.2.47 (dotnet#8761)
  ...
@grendello
Copy link
Contributor Author

Migrated to #8800

@grendello grendello closed this Mar 8, 2024
@grendello grendello deleted the blazor-hang branch March 8, 2024 12:31
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants