fix: crash problem with message_port close event

[wujinli]

in #22532 @nornagon add onclose method to MessagePort, but there is a crash scenario.
When worker_thread shutdown, it will destory context and close message_port. In this case, it should not dispatch close event. Because it forbid script running during NotifyContextDestroyed in ContextLifecycleNotifier.

We noticed that https://bugs.chromium.org/p/chromium/issues/detail?id=1495616 has started to implement the close event in Chromium, but it is not enabled by default yet. We still need to fix the crash. Once it's in a stable state we can use Chromium's implementation and remove this fix and #22532.

The crash stack is as follows:

frame #0: 0x00000001177aa59f Electron Framework blink::BeforeCallEnteredCallback(isolate=) at v8_per_isolate_data.cc:63:3 [opt]
frame #1: 0x0000000115f2f182 Electron Framework v8::CallDepthScope::CallDepthScope(v8::internal::Isolate*, v8::Localv8::Context) at isolate-inl.h:165:5 [opt]
frame #2: 0x0000000115f2f16c Electron Framework v8::CallDepthScope::CallDepthScope(this=, isolate=, context=) at api-inl.h:162:32 [opt]
frame #3: 0x0000000115f2e832 Electron Framework v8::Function::NewInstanceWithSideEffectType(v8::Localv8::Context, int, v8::Localv8::Value, v8::SideEffectType) const [inlined] v8::CallDepthScope::CallDepthScope(this=0x00007ff7b3ee2950, isolate=, context=(val_ = 0x00007fd37b02f060)) at api-inl.h:149:60 [opt]
frame #4: 0x0000000115f2e827 Electron Framework v8::Function::NewInstanceWithSideEffectType(this=0x00007fd37b02f050, context=(val_ = 0x00007fd37b02f060), argc=0, argv=0x0000000000000000, side_effect_type=kHasNoSideEffect) const at api.cc:5267:3 [opt]
frame #5: 0x0000000115880f74 Electron Framework blink::V8ObjectConstructor::NewInstance(isolate=0x00007fd370030000, function=(val_ = 0x00007fd37b02f050), argc=0, argv=0x0000000000000000) at v8_object_constructor.cc:51:49 [opt]
frame #6: 0x0000000115880dcc Electron Framework blink::V8PerContextData::CreateWrapperFromCacheSlowCase(this=0x0000010300226e98, type=0x000000011e5c56e0) at v8_per_context_data.cc:109:7 [opt]
frame #7: 0x00000001158808d1 Electron Framework blink::V8DOMWrapper::CreateWrapper(blink::ScriptState, blink::WrapperTypeInfo const*) [inlined] blink::V8PerContextData::CreateWrapperFromCache(this=, type=0x000000011e5c56e0) at v8_per_context_data.h:80:18 [opt]
frame #8: 0x00000001158808b7 Electron Framework blink::V8DOMWrapper::CreateWrapper(script_state=0x0000010300226e50, type=0x000000011e5c56e0) at v8_dom_wrapper.cc:57:33 [opt]
frame #9: 0x0000000115880349 Electron Framework blink::ScriptWrappable::Wrap(this=0x0000010300228630, script_state=0x0000010300226e50) at script_wrappable.cc:28:8 [opt]
frame #10: 0x0000000115957700 Electron Framework blink::JSBasedEventListener::Invoke(blink::ExecutionContext*, blink::Event*) [inlined] blink::ToV8(impl=0x0000010300228630, creation_context=, isolate=0x00007fd370030000) at to_v8.h:47:19 [opt]
frame #11: 0x000000011595762a Electron Framework blink::JSBasedEventListener::Invoke(this=0x0000010300227618, execution_context_of_event_target=, event=0x0000010300228630) at js_based_event_listener.cc:117:7 [opt]
frame #12: 0x0000000115c998ab Electron Framework blink::EventTarget::FireEventListeners(this=, event=0x0000010300228630, d=, entry=0x00000103001e5fd0) at event_target.cc:897:15 [opt]
frame #13: 0x00000001158ecfbb Electron Framework blink::EventTarget::FireEventListeners(this=0x00000103002189c0, event=0x0000010300228630) at event_target.cc:810:29 [opt]
frame #14: 0x0000000115f37740 Electron Framework blink::EventTarget::DispatchEventInternal(this=0x00000103002189c0, event=0x0000010300228630) at event_target.cc:717:41 [opt]
frame #15: 0x0000000116c69c94 Electron Framework blink::MessagePort::close(this=0x00000103002189c0) at message_port.cc:0 [opt]
frame #16: 0x0000000115d75b2f Electron Framework blink::ContextLifecycleObserver::NotifyContextDestroyed(this=) at context_lifecycle_observer.cc:46:3 [opt]
frame #17: 0x0000000115d75a58 Electron Framework blink::ContextLifecycleNotifier::NotifyContextDestroyed() [inlined] blink::ContextLifecycleNotifier::NotifyContextDestroyed()::$_0::operator()(this=, observer=) const at context_lifecycle_notifier.cc:37:15 [opt]
frame #18: 0x0000000115d75a53 Electron Framework blink::ContextLifecycleNotifier::NotifyContextDestroyed() at heap_observer_set.h:68:7 [opt]
frame #19: 0x0000000115d75a34 Electron Framework blink::ContextLifecycleNotifier::NotifyContextDestroyed(this=0x0000010300215868) at context_lifecycle_notifier.cc:36:14 [opt]
frame #20: 0x0000000115d758fb Electron Framework blink::LocalDOMWindow::FrameDestroyed(this=0x0000010300215868) at local_dom_window.cc:951:3 [opt]
frame #21: 0x0000000115d757be Electron Framework blink::LocalDOMWindow::Reset(this=) at local_dom_window.cc:964:3 [opt]
frame #22: 0x0000000116489d7b Electron Framework blink::LocalFrame::SetDOMWindow(this=0x000001030020d040, dom_window=0x000001030021c608) at local_frame.cc:843:18 [opt]
frame #23: 0x00000001169f3ce6 Electron Framework blink::DocumentLoader::InitializeWindow(this=0x000001030021b088, owner_document=) at document_loader.cc:2281:13 [opt]
frame #24: 0x00000001163a3b4c Electron Framework blink::DocumentLoader::CommitNavigation(this=0x000001030021b088) at document_loader.cc:2386:3 [opt]
frame #25: 0x00000001165393c7 Electron Framework blink::FrameLoader::CommitDocumentLoader(this=0x000001030020d1f0, document_loader=, previous_history_item=0x00000103002151b0, commit_reason=) at frame_loader.cc:1377:21 [opt]
frame #26: 0x000000011b847da6 Electron Framework blink::FrameLoader::CommitNavigation(this=0x000001030020d1f0, navigation_params=unique_ptr<blink::WebNavigationParams, std::Cr::default_deleteblink::WebNavigationParams > @ 0x00007ff7b3ee37b0, extra_data=unique_ptr<blink::WebDocumentLoader::ExtraData, std::Cr::default_deleteblink::WebDocumentLoader::ExtraData > @ 0x00007ff7b3ee37a8, commit_reason=kRegular) at frame_loader.cc:1212:3 [opt]
frame #27: 0x000000011b53999f Electron Framework blink::WebLocalFrameImpl::CommitNavigation(this=, navigation_params=unique_ptr<blink::WebNavigationParams, std::Cr::default_deleteblink::WebNavigationParams > @ 0x00007ff7b3ee3868, extra_data=) at web_local_frame_impl.cc:2610:24 [opt]

Reproduce the crash：

• 1、open crash demo(index.html)
• 2、when page load finished, click reload
• 3、You will find crash event in Renderer

// index.html

<script>
var sharedWorker = new SharedWorker('worker.js');

sharedWorker.port.addEventListener('close', function(event) {
    console.log('close event', event.data);
});
</script>

// worker.js

self.addEventListener('connect', function(event) {
  var port = event.ports[0];

  port.addEventListener('message', function(event) {
    console.log('Message from main:', event.data);
    port.postMessage('Hello from SharedWorker!');
});

});

Description of Change

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation, tutorials, templates and examples are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed crash in MessagePort::close.

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[deepak1556]

Thanks for the detailed context, looking at the upstream bug the close event has been enabled by default since 122. We should remove https://github.com/electron/electron/blob/main/patches/chromium/feat_add_onclose_to_messageport.patch in favor it for main and 29-x-y branch.

[wujinli]

https://bugs.chromium.org/p/chromium/issues/detail?id=1495616

@deepak1556 Okay, so should I submit a new PR to adapt to this change? Or will other official personnel handle it?

[deepak1556]

You can make the changes in this PR since the test you have added is good to have. So next steps would be,

• Delete https://github.com/electron/electron/blob/main/patches/chromium/feat_add_onclose_to_messageport.patch
• Delete the new patch you have added fix_crash_problem_with_message_port_close_event.patch
• Keep the tests and they should potentially pass now as well

[wujinli]

@deepak1556 Thanks for your reply. I'm done. After testing, it can pass the test of 'ipc module', which means that the function of close_event is not damaged. Also, I added a test to make sure the same crash doesn't occur.

[wujinli]

@deepak1556 Will it automatically merge in? If not, it seems like I don't have the authority to operate it. Could you please help with the operation.

[deepak1556]

@electron/patch-owners need another review before merging.

[wujinli]

@deepak1556 Thanks for your reply. @codebytere Can you help review the code?

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉

[release-clerk]

Release Notes Persisted

Fixed crash in MessagePort::close.

[trop]

I have automatically backported this PR to "29-x-y", please check out #41237