Android ResolveRequestedModules? crash in [@ xpc::NativeGlobal]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: cpeterson, Assigned: jonco)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
|
Details | Review |
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
Updated•1 year ago
|
Comment 1•1 year ago
|
||
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.
Comment 2•1 year ago
|
||
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.
Assignee | ||
Comment 3•1 year ago
|
||
(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.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 4•1 year ago
|
||
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
Comment 6•1 year ago
|
||
bugherder |
Comment 7•1 year ago
|
||
Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.
Reporter | ||
Comment 8•1 year ago
|
||
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?
Comment 9•1 year ago
•
|
||
: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.
Comment 10•1 year ago
|
||
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.
Comment 11•1 year ago
|
||
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?
Comment 12•1 year ago
|
||
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?
Comment 13•1 year ago
|
||
(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.
Comment 14•1 year ago
|
||
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
Comment 15•1 year ago
•
|
||
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
Comment 16•1 year ago
|
||
bugherder uplift |
Description
•