Skip to content

Support for POST requests and setting request headers - #12

Merged
bghgary merged 17 commits into
BabylonJS:mainfrom
lmaskati:lmaskati/test
Mar 9, 2023
Merged

Support for POST requests and setting request headers#12
bghgary merged 17 commits into
BabylonJS:mainfrom
lmaskati:lmaskati/test

Conversation

@lmaskati

@lmaskatilmaskati commented Mar 6, 2023

Copy link
Copy Markdown
Contributor

This PR is a draft until BabylonJS/UrlLib#1 is merged, after which it needs to be tested with an updated CMake.

bghgary
bghgary reviewed Mar 6, 2023
View reviewed changes

@bghgarybghgary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass. Will review again after updates.

Comment threadCMakeLists.txt
Comment threadCMakeLists.txt Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadTests/UnitTests/Scripts/tests.js Outdated
Comment threadTests/UnitTests/Scripts/tests.js Outdated
@bghgary

bghgary commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

PR title could be updated here too :-)

@lmaskatilmaskati changed the title Lmaskati/testSupport for POST requests and setting request headersMar 8, 2023
@lmaskati
lmaskati marked this pull request as ready for review March 8, 2023 19:04
bghgary
bghgary reviewed Mar 8, 2023
View reviewed changes

@bghgarybghgary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some small things. Looking good otherwise.

Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
Comment threadPolyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Outdated
@bghgary
bghgary merged commit c0637d5 into BabylonJS:mainMar 9, 2023
@bkaradzic-microsoftbkaradzic-microsoft mentioned this pull request Jun 2, 2026
Merged
CedricGuillemet added a commit to CedricGuillemet/JsRuntimeHost that referenced this pull request Jun 3, 2026
@CedricGuillemet
Hermes: patch shutdown() to fix napi_ref double-free during teardown
047f373
Captured a full backtrace via lldb -k (see prior CI commit). The smoking gun:
frame BabylonJS#5: ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED
frame BabylonJS#6: napi_delete_reference at hermes_napi_reference.cpp:113
frame BabylonJS#7: Napi::Reference<...>::~Reference at napi-inl.h:3262
frame BabylonJS#8: Napi::ObjectReference::~ObjectReference
frame BabylonJS#10: Babylon::Polyfills::Internal::URL::~URL at URL.h:10
frame BabylonJS#12: Napi::ObjectWrap<...URL>::FinalizeCallback at napi-inl.h:4963
frame BabylonJS#13: napi_env__::shutdown at hermes_napi.cpp:214
frame BabylonJS#16: hermes::vm::Runtime::~Runtime
Root cause: Hermes's napi_env__::shutdown() iterates refListHead_ and delete ref; one at a time. It only sets ref->deletionPending_ on the *current* ref before its finalize_cb fires. If the finalizer transitively destroys a node-addon-api wrapper (Napi::Reference / Napi::ObjectReference) whose underlying napi_ref was already deleted earlier in the same loop, napi_delete_reference reads ref->deletionPending_ from freed memory and proceeds to delete ref again -> double-free.
The exact path: URL (an ObjectWrap subclass) has a Napi::ObjectReference member m_searchParamsReference. addReference prepends to the linked list, so m_searchParamsReference's ref is processed BEFORE URL's wrap ref. When URL's wrap finalizer runs delete this, ~URL destroys m_searchParamsReference, whose destructor calls napi_delete_reference on the already-freed sibling ref.
macOS libmalloc detects this (malloc: *** error for object 0x...: pointer being freed was not allocated -> SIGABRT). Linux glibc and Windows CRT happen to miss it.
Fix: PATCH Hermes shutdown() to mark ALL refs deletionPending in a pre-pass BEFORE iterating. Apply as a FetchContent PATCH_COMMAND via the new ApplyPatchIfNeeded.cmake helper (idempotent — uses git apply --check --reverse to detect already-applied state across reconfigure).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bghgarybghgarybghgary approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lmaskati@bghgary