1916569 - Crash in [@ mozilla::ExtensionPolicyService::ExecuteContentScripts]

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[BugBot [:suhaib / :marco/ :calixte]]

Crash report: https://crash-stats.mozilla.org/report/index/de1580dc-4554-4c9e-8d9f-598700240903

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(promise)

Top 10 frames of crashing thread:

0  xul.dll  mozilla::ExtensionPolicyService::ExecuteContentScripts  toolkit/components/extensions/ExtensionPolicyService.cpp:355
1  xul.dll  mozilla::ExtensionPolicyService::InjectContentScripts::<lambda_1>::operator const  toolkit/components/extensions/ExtensionPolicyService.cpp:426
1  xul.dll  mozilla::dom::  dom/promise/Promise-inl.h:206
1  xul.dll  mozilla::dom::  dom/promise/Promise-inl.h:214
1  xul.dll  mozilla::dom::  dom/promise/Promise-inl.h:185
2  xul.dll  mozilla::dom::PromiseNativeThenHandlerBase::ResolvedCallback  dom/promise/Promise.cpp:241
3  xul.dll  mozilla::dom::  dom/promise/Promise.cpp:416
4  xul.dll  JS::detail::CallArgsBase<JS::detail::IncludeUsedRval>::get const  js/public/CallArgs.h:222
4  xul.dll  mozilla::dom::NativeHandlerCallback  dom/promise/Promise.cpp
5  xul.dll  CallJSNative  js/src/vm/Interpreter.cpp:480

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2024-07-02
• Process type: Content
• Is startup crash: No
• Has user comments: No
• Is null crash: No

Comment 1

[Rob Wu [:robwu]]

This assertion is triggered at https://searchfox.org/mozilla-central/rev/c414b4538dd3c7e1dc674f7b66176e7c309afa95/toolkit/components/extensions/ExtensionPolicyService.cpp#364

Promise::All is called after calling ExecuteContentScript, which may execute arbitrary JS from an extension. Consequently it is possible for the underlying global to somehow be destroyed.

We should therefore not assert that promise is not nullptr (since it clearly can). If possible we could return the nullptr; if a Promise is required we could also return a rejected promise. It is not immediately clear to me how we could create a rejected promise under these circumstances though...

:baku What would you recommend here?

Comment 2

[Andrea Marchesini [:baku]]

I prefer Smaug to answer here. But to me what happens is the following:

1. one of the content script destroys the global.
2. after that, any calls to ExecuteContentScript should return an nullptr: https://searchfox.org/mozilla-central/rev/c414b4538dd3c7e1dc674f7b66176e7c309afa95/toolkit/components/extensions/ExtensionPolicyService.cpp#358-359
3. if Promise:All returns nullptr I suspect that at that point does not really matter the promise we return. We can actually return any of the promises we have in the promises array. This is similar to what happens here: https://searchfox.org/mozilla-central/source/dom/promise/Promise.cpp#193-198. They will become no-op.

Smaug, thoughts?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I don't really know anything about extensions.
I could note that Promise::CreateInfallible can be useful in some cases when error handling is tricky

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Bug 1916569 - Fix a crash in mozilla::ExtensionPolicyService::ExecuteContentScripts, r?rpl

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/644be0ced4f4
Fix a crash in mozilla::ExtensionPolicyService::ExecuteContentScripts, r=rpl

Comment 6

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/644be0ced4f4

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:baku, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox133 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Andrea Marchesini [:baku]]

The number of crashes is low, I would not uplift this fix.