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

initialize Performance::timeOrigin in create #158

Closed
wants to merge 1 commit into from

Conversation

cataggar
Copy link

@cataggar cataggar commented Oct 12, 2024

I'm running into a panic in StarlingMonkey when I run ComponentizeJS. It looks like timeOrigin is not being initialized. I'm not sure if this is the correct fix, but it makes ComponentizeJS work for me. From bytecodealliance/ComponentizeJS#138 (comment):

~/ms/ComponentizeJS> npx jco componentize /Users/cataggar/ms/StarlingMonkey15/cowsay/cowsay.js --wit /Users/cataggar/ms/StarlingMonkey15/cowsay/src/cowsay.wit -o /Users/cataggar/ms/StarlingMonkey15/cowsay.wasm
Redirecting call to abort() to mozalloc_abort

Error: the `componentize.wizer` function trapped

Caused by:
    0: error while executing at wasm backtrace:
           0: 0x16409 - mozalloc_abort
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/obj-release/dist/include/mozilla/Assertions.h:58:3
           1: 0x16412 - abort
           2: 0xd9f3 - std::__2::__throw_bad_optional_access[abi:v160000]()
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/deps/cpm_cache/wasi-sdk/c3b77b0f4b7d10c88fc0dd93d5dae49796959c98/wasi-sdk/bin/../share/wasi-sysroot/include/c++/v1/optional:214:9
           3: 0x3364c - std::__2::optional<std::__2::chrono::time_point<std::__2::chrono::steady_clock, std::__2::chrono::duration<long long, std::__2::ratio<(long long)1, (long long)1000000000> > > >::value[abi:v160000]() &
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/deps/cpm_cache/wasi-sdk/c3b77b0f4b7d10c88fc0dd93d5dae49796959c98/wasi-sdk/bin/../share/wasi-sysroot/include/c++/v1/optional:1004:13              - builtins::web::performance::Performance::timeOrigin_get(JSContext*, unsigned int, JS::Value*)
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/builtins/web/performance.cpp:30:54
           4: 0x267638 - CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:488:7              - js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:581:12
           5: 0x268076 - js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:680:7
           6: 0x268787 - js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:802:10
           7: 0x395750 - <unknown>!js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>)
           8: 0x26b6e8 - js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/ObjectOperations-inl.h:117:10              - js::ToObjectFromStackForPropertyAccess(JSContext*, JS::Handle<JS::Value>, int, JS::Handle<js::PropertyName*>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/ObjectOperations-inl.h:124:10              - js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:4730:11
           9: 0x25ff99 - <unknown>!js::Interpret(JSContext*, js::RunState&)
          10: 0x25a84b - MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:401:10              - js::RunScript(JSContext*, js::RunState&)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:459:13
          11: 0x268b27 - js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:846:13              - js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Interpreter.cpp:878:10
          12: 0x2b34f7 - js::ModuleObject::execute(JSContext*, JS::Handle<js::ModuleObject*>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/builtin/ModuleObject.cpp:1420:10
          13: 0x38a4f7 - InnerModuleEvaluation(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::GCVector<js::ModuleObject*, (unsigned long)0, js::SystemAllocPolicy> >, unsigned long, unsigned long*)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Modules.cpp:1752:9
          14: 0x38a3a6 - InnerModuleEvaluation(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::GCVector<js::ModuleObject*, (unsigned long)0, js::SystemAllocPolicy> >, unsigned long, unsigned long*)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Modules.cpp:1669:9
          15: 0x389fbf - ModuleEvaluate(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Modules.cpp:1523:13              - JS::ModuleEvaluate(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)
                           at /home/runner/work/spidermonkey-wasi-embedding/spidermonkey-wasi-embedding/gecko-dev/js/src/vm/Modules.cpp:214:10
          16: 0x10eb5 - ScriptLoader::eval_top_level_script(char const*, JS::SourceText<mozilla::Utf8Unit>&, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>)
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/runtime/script_loader.cpp:497:8
          17: 0xd800 - <unknown>!api::Engine::eval_toplevel(JS::SourceText<mozilla::Utf8Unit>&, char const*, JS::MutableHandle<JS::Value>)
          18: 0xd56b - api::Engine::eval_toplevel(char const*, JS::MutableHandle<JS::Value>)
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/runtime/engine.cpp:528:10              - api::Engine::Engine(std::__2::unique_ptr<api::EngineConfig, std::__2::default_delete<api::EngineConfig> >)
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/runtime/engine.cpp:410:10
          19: 0xbcd7 - wizen()
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/runtime/js.cpp:38:16
          20: 0xbe7a - __wizer_initialize()
                           at /Users/cataggar/ms/ComponentizeJS/StarlingMonkey/runtime/js.cpp:42:1
          21: 0x234c4e - std::__2::vector<componentize::embedding::CoreVal, std::__2::allocator<componentize::embedding::CoreVal> >::push_back[abi:v160000](componentize::embedding::CoreVal&&)
                           at /Users/cataggar/ms/ComponentizeJS/embedding/embedding.cpp:353:3              - componentize_initialize
                           at /Users/cataggar/ms/ComponentizeJS/embedding/embedding.cpp:395:18
    1: wasm trap: wasm `unreachable` instruction executed
(jco componentize) Error: Failed to initialize the compiled Wasm binary with Wizer:
Wizering failed to complete
    at componentize (file:///Users/cataggar/ms/ComponentizeJS/src/componentize.js:271:13)
    at async componentize (file:///Users/cataggar/ms/jco/src/cmd/componentize.js:11:25)
    at async file:///Users/cataggar/ms/jco/src/jco.js:200:9

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

I initially thought this was an innocent little change that obviously makes sense. Unfortunately further reflection reveals that it's more subtle.

Taking a step back: yes, we should absolutely enable use of timeOrigin and things based on it during initialization, instead of just in event handlers. (Currently, it's only set by the FetchEvent handler, at least inside of StarlingMonkey itself.)

The problem is what to do in situations where we want to use it both during initialization and in event handlers. I can see three possible answers:

  1. We always return 0 for it, as CloudFlare Workers do
  2. We set it during initialization and at the start of the event handler
  3. We set it once during initialization and then keep that value

With this patch, we'd choose the second of these options. I think that runs the risk of showing very confusing or perhaps even fatally wrong results: if timeOrigin (or any value derived from it) is used to compute something during initialization, and that then is used together with the new timeOrigin to compute a new value in an event handler, the results could be off or even something worse, such as negative time spans. (A trivial example would be storing the value of performance.now() during initialization, and then subtracting that from performance.now() in an event handler.)

I'm leaning towards the first option here instead, but that requires careful evaluation of how we should handle performance.now and other potential derived values. We definitely need to guarantee monotonicity for that, so we might want to store the duration of initialization and add that to whatever performance.now() would return in an event handler.

I very much understand if this is all much more than you bargained for when opening this PR, @cataggar, but if you're interested, I'm more than happy to collaborate on getting this all sorted.

@guybedford
Copy link
Contributor

I think each embedder should probably configure its own behaviour here according to its execution model.

For Fastly, we also set this on the request handler only.

But to fix this in ComponentizeJS I've posted a ComponentizeJS-specific issue in bytecodealliance/ComponentizeJS#144.

@cataggar
Copy link
Author

@tschneidereit, thanks for the in-depth response. I'm going to close this in favor of it being fixed downstream in bytecodealliance/ComponentizeJS#144.

@cataggar cataggar closed this Oct 18, 2024
@cataggar cataggar deleted the timeOrigin-init branch October 18, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants