Closed Bug 1797166 Opened 1 year ago Closed 1 year ago

Android ResolveRequestedModules? crash in [@ xpc::NativeGlobal]

Categories

(Core :: JavaScript Engine, defect)

All
Android
defect

Tracking

()

RESOLVED FIXED
108 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox106 --- wontfix
firefox107 --- fixed
firefox108 --- fixed

People

(Reporter: cpeterson, Assigned: jonco)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

This crash looks similar to ResolveRequestedModules bug 1342810 that was fixed six years ago.

Crash report: https://crash-stats.mozilla.org/report/index/f7b6597b-e597-41ec-ac6f-ae25a0221024

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so xpc::NativeGlobal js/xpconnect/wrappers/WrapperFactory.cpp:778
1 libxul.so mozilla::dom::AutoJSAPI::Init dom/script/ScriptSettings.cpp:431
2 libxul.so JS::loader::ModuleLoaderBase::ResolveRequestedModules js/loader/ModuleLoaderBase.cpp:649
3 libxul.so JS::loader::ModuleLoaderBase::StartFetchingModuleDependencies js/loader/ModuleLoaderBase.cpp:725
4 libxul.so JS::loader::ModuleLoaderBase::OnFetchComplete js/loader/ModuleLoaderBase.cpp:463
5 libxul.so mozilla::dom::ScriptLoadHandler::OnStreamComplete dom/script/ScriptLoadHandler.cpp:460
6 libxul.so nsIncrementalStreamLoader::OnStopRequest netwerk/base/nsIncrementalStreamLoader.cpp:82
7 libxul.so nsCORSListenerProxy::OnStopRequest netwerk/protocol/http/nsCORSListenerProxy.cpp:677
8 libxul.so std::__ndk1::__function::__func<mozilla::net::HttpChannelChild::ProcessOnStopRequest /builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include/functional:1714
9 libxul.so mozilla::net::ChannelEventQueue::FlushQueue netwerk/ipc/ChannelEventQueue.cpp:94
Component: DOM: Core & HTML → JavaScript Engine

The bug is linked to a topcrash signature, which matches the following criterion:

  • Top 10 content process crashes on beta

:sdetar, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sdetar)
Keywords: topcrash

These crashes have gone away in 106 because we have more precise backtraces, but searching for xpc::NativeGlobal in the proto signature still turns up plenty of crashes post-106. Looking at the first 10 crashes on that list, they all call AutoJSAPI::Init with an object and crash with a nullptr deref as soon as we try accessing that object's shape field.

There seem to be at least three places that call Init, all of which are module related: ModuleLoaderBase::FinishDynamicImportAndReject, ModuleLoaderBase::InstantiateModuleGraph, and ModuleLoaderBase::ResolveRequestedModules.

Jon, does any of this look actionable? It looks like you touched this code most recently.

Flags: needinfo?(jcoppeard)

(In reply to Iain Ireland [:iain] from comment #2)
Hopefully the crashes related to dynamic import were fixed by bug 1712762 which went into 107.

The crashes involving ResolveRequestedModules and InstantiateModuleGraph are both caused by a ModuleScript having a null ModuleObject but also no parse error set.

Flags: needinfo?(jcoppeard)
Assignee: nobody → jcoppeard

It would be nice to assume this but CompileFetchedModule can potentially fail for reasons that
are not JS-related.

Also check for the exception being |undefined| which would cause
ModuleScript::HasParseError to return false. Such an exception should never be
thrown by parsing but check for it just in case.

Add a diagnostic assert to check module script state is as we expect to
hopefully catch related problems sooner.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de0d313e0b1a
Don't assume an exception is always set if module compilation fails r=yulia
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch

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

I think it's too late to uplift this fix to late Beta 107. We only have one more Beta build before 107's RC freeze.

Would this fix be a good candidate to ride along in a 107 dot release?

:iain sorry for the needinfo, I see :jonco is on currently on PTO.
Could you clarify Comment 2 and Comment 3, which signatures to look out for in 107 beta if we are still seeing significant problems?
Would like to consider uplifting this to 107 before we ship if it's needed. The final 107 beta builds tomorrow.

Flags: needinfo?(iireland)

Setting 107 to affected, to keep this on the radar for 107
If not uplifted to 107, at least it may go in a dot release. Pending response to Comment 9.

With the proviso that I don't know this code very well: it looks to me like the fix targeting crashes with ModuleLoaderBase::FinishDynamicImportAndReject in the proto signature landed in bug 1712762. This patch targets the other crashes with xpc::NativeGlobal in the proto signature.

This query looking at xpc::NativeGlobal crashes over the last week shows a lot of crashes in 106, but only one crash in 107. Oddly, the crash in 107 is a FinishDynamicImportAndReject crash, which bug 1712762 was supposed to address.

Picking on the patch reviewer: Yulia, do you think it makes sense to uplift this to 107 now?

Flags: needinfo?(iireland) → needinfo?(ystartsev)

The rare crashes on 107 make me less worried about this, but also I think it would be fine to uplift this. If necessary I can do the uplift request?

Flags: needinfo?(ystartsev)

(In reply to Yulia Startsev [:yulia] from comment #12)

The rare crashes on 107 make me less worried about this, but also I think it would be fine to uplift this. If necessary I can do the uplift request?

Thanks, Yulia. I think it makes sense to take this in 107. If you could add the beta uplit request, I should be able to get this in before the final 107 beta build today.

Flags: needinfo?(ystartsev)

Comment on attachment 9301108 [details]
Bug 1797166 - Don't assume an exception is always set if module compilation fails r?yulia

Beta/Release Uplift Approval Request

  • User impact if declined: There may be unexpected crashes for users when visiting sites using modules. Modules are used on appx. 9% of pages according to chrome stats: https://chromestatus.com/metrics/feature/timeline/popularity/2615
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is not risky as it has been checked on nightly with no regressions.
  • String changes made/needed: None
  • Is Android affected?: Yes
Flags: needinfo?(ystartsev)
Attachment #9301108 - Flags: approval-mozilla-beta?

Comment on attachment 9301108 [details]
Bug 1797166 - Don't assume an exception is always set if module compilation fails r?yulia

Approved for 107.0b9 and Fenix/Focus 107.0b6

Attachment #9301108 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: