Fix wasm null JS event handlers before release#3432
Conversation
| if d.onCloseHandler != nil { | ||
| d.underlying.Set("onclose", js.Null()) | ||
| d.onCloseHandler.Release() | ||
| } | ||
| if d.onClosingHandler != nil { | ||
| d.underlying.Set("onclosing", js.Null()) | ||
| d.onClosingHandler.Release() | ||
| } |
There was a problem hiding this comment.
Hello, clearing onClose and onError this early will prevent us from receiving the close event later. We'll need to fix this before this is merged, thank you.
|
Seems like the CI is stuck for some reason, That's weird. |
|
It's also stuck for other PRs pion/mdns#275 seems like a github issue... |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3432 +/- ##
==========================================
- Coverage 85.50% 85.47% -0.04%
==========================================
Files 81 81
Lines 9857 9857
==========================================
- Hits 8428 8425 -3
- Misses 1010 1012 +2
- Partials 419 420 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alinz
changed the title
alinz
changed the title
| if d.onCloseHandler != nil { | ||
| d.onCloseHandler.Release() | ||
| } |
There was a problem hiding this comment.
I think we should defer nullifying and releasing onclose/onerror after close is triggered,.
There was a problem hiding this comment.
Thanks for the feedback, It got a little bit more complicated than I thought, but please have a look at my implementation here
| wrapperHandler = js.FuncOf(func(this js.Value, args []js.Value) any { | ||
| if closeFunc != nil { | ||
| go closeFunc() | ||
| } | ||
| d.underlying.Set("onclose", js.Null()) | ||
| wrapperHandler.Release() | ||
| if oldErrorHandler != nil { | ||
| d.underlying.Set("onerror", js.Null()) | ||
| oldErrorHandler.Release() | ||
| } | ||
| return js.Undefined() | ||
| }) | ||
| d.underlying.Set("onclose", wrapperHandler) | ||
| if oldCloseHandler != nil { | ||
| oldCloseHandler.Release() | ||
| } | ||
| d.onCloseHandler = nil | ||
| d.onCloseFunc = nil | ||
| d.onErrorHandler = nil |
There was a problem hiding this comment.
I think before we merge this we just need to make sure we don't override the previous close if the user calls close twice or more for whatever reason.
Thank you.
alinz
changed the title
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
Thank you and sorry for the back and forth.
| // close() and the onclose event), and the user-provided OnClose callback | ||
| // must still be invoked. Install the wrapper only once so repeated Close() | ||
| // calls don't override the original wrapper before it fires. | ||
| if !d.closeWrapperInstalled { |
There was a problem hiding this comment.
Nit: we need to avoid installing the wrapper if already closed., but we can do this later. I think.
alinz
changed the title
alinz
changed the title
|
My apologies @JoTurk , but the Lint Meta data keeps failing, and I don't have any clue how to solve this. |
Thank you for your fixes. the CI is failing because we have lint rules for commit messages, but it doesn't matter I'm going to squash merge it. Thank you again. |
2 participants
I ran into a frustrating bug when using pion/webrtc in a WASM app:
after closing a peer connection, every subsequent operation would
blow up with syscall/js: call to released function in the browser
console. Took a while to track down.