TracingPlatform performs undefined behavior in JS that makes it unusable in Wasm

[sjrd]

cats-effect/core/js/src/main/scala/cats/effect/tracing/TracingPlatform.scala

Lines 28 to 31 in 18fdf52

	private[this] val function0Property =
	js.Object.getOwnPropertyNames((() => ()).asInstanceOf[js.Object])(0)
	private[this] val function1Property =
	js.Object.getOwnPropertyNames(((_: Unit) => ()).asInstanceOf[js.Object])(0)

These lines attempt to break through Scala.js abstractions. They access JS properties of a Scala.js object. That is undefined behavior.

When linking for WebAssembly, they actually crash, since Scala.js objects really don't have JS properties.

Since Tracing is always called by IO.async, it makes the latter completely unusable with Scala.js-on-Wasm.

There does not appear to be any workaround.

[durban]

This is probably the same issue as #4608. If I understand it correctly, this code in TracingPlatform is also incorrect in regular-JS, and not just WASM. (It just "happens to work" on regular-JS, and crashes on WASM.) That is good to know. Maybe @armanbilge remembers why was it written this way originally. /cc @samspills

[sjrd]

If I understand it correctly, this code in TracingPlatform is also incorrect in regular-JS, and not just WASM. (It just "happens to work" on regular-JS, and crashes on WASM.)

Yes, you understand correctly :)

[armanbilge]

Maybe @armanbilge remembers why was it written this way originally.

I was aware that this breaks abstractions and relies on implementation details to work. This was the only way I could figure out how to support cached tracing on Scala.js on JavaScript when I worked on it 5 years ago in #2207. (Apologies for not documenting as a code comment.)

TracingPlatform is also incorrect in regular-JS

I'm not sure what you mean by this.

---

If there is a way to support cached tracing on Scala.js-on-JS without relying on undefined behavior and implementation details, we should definitely do that. In any case, we should definitely implement this in a way that does not prevent the use of the Wasm platform.

[sjrd]

TracingPlatform is also incorrect in regular-JS

I'm not sure what you mean by this.

That it only "happens to work" given the current implementation of Scala.js. Any upgrade of Scala.js could break that abstraction breach without even a release note item.

If there is a way to support cached tracing on Scala.js-on-JS without relying on undefined behavior and implementation details, we should definitely do that. In any case, we should definitely implement this in a way that does not prevent the use of the Wasm platform.

Could you describe, or point me to, what that feature is trying to achieve in the first place?

[djspiewak]

@sjrd Looking at the JVM implementation of the same trait is probably the clearest way to piece together the intent. The full guts of what's going on here…

When users call methods like map or flatMap or similar, we want to capture the call site in order to build a trace frame. This in turn is what allows us to implement fiber tracing, which underlies Cats Effect's enhanced exceptions and fiber dumps. Without this functionality, every exception occurring within Cats Effect would have almost exactly the same useless stack trace due to the trampoline. There are, to the best of my knowledge, three ways of implementing this type of functionality:

• Implicit evidence. Used by Kyo, ZIO 2, Munit, and others
• Bytecode introspection. Used by Akka and ZIO 1
• Exception stack trace walking. Used by Specs 1 (and 2 in many cases), Scala Test (in most cases), JUnit (and most other Java test frameworks), and Cats Effect 2 and 3

All of these have some serious tradeoffs. The implicit evidence is the most pleasing from a performance standpoint, but it pollutes type signatures and defies abstraction (e.g. Monad#flatMap has no such evidence). Bytecode introspection gives you the most flexibility on semantics, but it's very expensive, doesn't always yield the answer the user expects, and can run you into some interesting issues when users have custom classloaders or agents. Stack walking is nice because it gives you the precise runtime call path (similar to implicit evidence), and in fact can give you the whole call path allowing for a sort of two-dimensional stack trace, but it's extremely expensive and requires some careful allowlisting to avoid unintuitive artifacts.

The main tradeoff of the latter two options is runtime cost, since in theory this computation needs to be performed on every call to flatMap (and similar). ZIO 1, CE2, and CE3 all take advantage of the same trick in order to mitigate this problem: caching.

The trick is that flatMap (and similar) are usually invoked with call-site-specific closures, meaning that we can inspect the runtime Class of the Function1 to derive a cache key which is unique to the static call site. With a little bit of shenaniganry, we can even do the same thing for by-name functions, allowing us to extend tracing coverage a bit. Since getClass is extremely cheap on the JVM, and the number of call sites in an application is generally very bounded, the cache is surprisingly small and the hit rate almost immediately reaches 100% once the application is warmed.

The problem is that function values do not have classes on Scala.js. :D The code you're looking at is designed to work around this by fishing out a different property which happens to be call-site specific and efficient to access.

So in other words, all of this is kind of straddling the line on behavior. On the one hand, it's really important functionality. Enhanced exceptions are genuinely one of our most important practical production features. On the other hand, even on the JVM this is technically relying on implementation details of the Scala bytecode representation. We're relatively comfortable relying on such implementation details if needed in this layer (which is why we engaged in this pretty obviously dodgy hackery in the first place), but obviously it has limits.

Very open to suggestions on better ways to achieve this same functionality.

[sjrd]

I don't think there is a user-space solution.

IMO, your best course of action would be a link-time IR-rewriting plugin. Wrap the ClearableLinker of Scala.js with a custom one. See
https://github.com/scala-js/scala-js/tree/main/sbt-plugin/src/sbt-test/linker/custom-linker
for an example.

In the plugin, override def link to patch the IR before shipping it to the standard Linker. Find all the NewLambda nodes that target AbstractFunction0 or AbstractFunction1 (potentially restricted to call sites of interest). Rewrite them to a New of a custom class that you add to your runtime. That class can hold a field with a call-site identity of your choosing. Since you now control the New node, you can inject the identity.

In your TracingPlatform runtime, type-test for your custom class. If it's an instance, fetch the identity field; otherwise use getClass() like on the JVM.

It is some amount of work, but it would be completely reliable.

[djspiewak]

That's not something users can incorporate as a library dependency though, which is a really significant tradeoff and well outside the normal Cats Effect adoption model. NGL, the center of mass in my thinking here still biases towards piling more hacks on top of the hacks and simply detecting WASM (and potential future releases which alter this encoding). The identity will always be there somewhere because of what closures fundamentally are. I'd certainly prefer something in userspace (or userspace-ish, like the JVM solution), but either way I don't think we can force users to import build plugins.

[sjrd]

On Wasm the lambda really does not have an identity. funcref is not a subtype of eqref, so you can't perform an identity test on a lambda.

Even on JS you are using toString(), which is not an identity. You could get the same "identity" for two different call sites if they happen to render to the same JS source code.