1645246 - Crash in nsXULPrototypeDocument::RebuildL10nPrototype (MOZ_RELEASE_ASSERT(proto)) when opening the migration wizard after changing some fluent stuff

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[:Gijs (he/him)]

Attachment: crashy-patch.txt

STR:

1. fresh m-c checkout + build
2. apply attached patch (https://phabricator.services.mozilla.com/D79383 at time of writing)
3. ./mach build faster
4. run Firefox
5. open migration wizard (file menu, help menu, or via the library window)

ER: not crash

AR: crash, with no crash reporter or console output

Crash stack:

>	xul.dll!nsXULPrototypeDocument::RebuildL10nPrototype(mozilla::dom::Element * aElement, bool aDeep) Line 525	C++
 	xul.dll!mozilla::dom::DOMLocalization::ApplyTranslations(nsTArray<nsCOMPtr<mozilla::dom::Element> > & aElements, nsTArray<mozilla::dom::Nullable<mozilla::dom::L10nMessage> > & aTranslations, nsXULPrototypeDocument * aProto, mozilla::ErrorResult & aRv) Line 0	C++
 	xul.dll!ElementTranslationHandler::ResolvedCallback(JSContext * aCx, JS::Handle<JS::Value>) Line 239	C++
 	xul.dll!mozilla::dom::`anonymous namespace'::PromiseNativeHandlerShim::ResolvedCallback(JSContext * aCx, JS::Handle<JS::Value> aValue) Line 388	C++
 	xul.dll!mozilla::dom::NativeHandlerCallback(JSContext * aCx, unsigned int aArgc, JS::Value * aVp) Line 339	C++
 	[Inline Frame] xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, js::CallReason reason, const JS::CallArgs & args) Line 486	C++
 	xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 578	C++
 	[Inline Frame] xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 641	C++
 	xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, const js::AnyInvokeArgs & args, JS::MutableHandle<JS::Value> rval, js::CallReason reason) Line 658	C++
 	[Inline Frame] xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, JS::Handle<JS::Value> arg0, JS::MutableHandle<JS::Value> rval) Line 104	C++
 	xul.dll!PromiseReactionJob(JSContext * cx, unsigned int argc, JS::Value * vp) Line 1906	C++
 	[Inline Frame] xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, js::CallReason reason, const JS::CallArgs & args) Line 486	C++
 	xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 578	C++
 	[Inline Frame] xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 641	C++
 	xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, const js::AnyInvokeArgs & args, JS::MutableHandle<JS::Value> rval, js::CallReason reason) Line 658	C++
 	xul.dll!JS::Call(JSContext * cx, JS::Handle<JS::Value> thisv, JS::Handle<JS::Value> fval, const JS::HandleValueArray & args, JS::MutableHandle<JS::Value> rval) Line 2842	C++
 	xul.dll!mozilla::dom::MozJSActorCallback::Call(mozilla::dom::BindingCallContext & cx, JS::Handle<JS::Value> aThisVal, mozilla::ErrorResult & aRv) Line 190	C++
 	[Inline Frame] xul.dll!mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult & aRv, const char * aExecutionReason, mozilla::dom::CallbackObject::ExceptionHandling aExceptionHandling, JS::Realm * aRealm) Line 91	C++
 	[Inline Frame] xul.dll!mozilla::dom::PromiseJobCallback::Call(const char * aExecutionReason) Line 104	C++
 	xul.dll!mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation & aAso) Line 209	C++
 	xul.dll!mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool aForce) Line 640	C++
 	xul.dll!mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int aRecursionDepth) Line 462	C++
 	xul.dll!XPCJSContext::AfterProcessTask(unsigned int aNewRecursionDepth) Line 1365	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1267	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 501	C++
 	[Inline Frame] xul.dll!mozilla::SpinEventLoopUntil(mozilla::AppWindow::ShowModal::<unnamed-tag> && aPredicate, nsIThread * aThread) Line 359	C++
 	xul.dll!mozilla::AppWindow::ShowModal() Line 505	C++
 	xul.dll!nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy * aParent, const char * aUrl, const char * aName, const char * aFeatures, bool aCalledFromJS, bool aDialog, bool aNavigate, nsIArray * aArgv, bool aIsPopupSpam, bool aForceNoOpener, bool aForceNoReferrer, nsDocShellLoadState * aLoadState, mozilla::dom::BrowsingContext * * aResult) Line 1309	C++
 	xul.dll!nsWindowWatcher::OpenWindow(mozIDOMWindowProxy * aParent, const char * aUrl, const char * aName, const char * aFeatures, nsISupports * aArguments, mozIDOMWindowProxy * * aResult) Line 292	C++
 	xul.dll!XPTC__InvokebyIndex()	Unknown
 	[Inline Frame] xul.dll!CallMethodHelper::Invoke() Line 1618	C++
 	[Inline Frame] xul.dll!CallMethodHelper::Call() Line 1174	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode) Line 1140	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 946	C++
 	[Inline Frame] xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, js::CallReason reason, const JS::CallArgs & args) Line 486	C++
 	xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 578	C++
 	[Inline Frame] xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 641	C++
 	[Inline Frame] xul.dll!js::CallFromStack(JSContext * cx, const JS::CallArgs & args) Line 645	C++
 	xul.dll!Interpret(JSContext * cx, js::RunState & state) Line 3300	C++
 	xul.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 458	C++
 	xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 613	C++
 	[Inline Frame] xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 641	C++
 	xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, const js::AnyInvokeArgs & args, JS::MutableHandle<JS::Value> rval, js::CallReason reason) Line 658	C++
 	xul.dll!JS::Call(JSContext * cx, JS::Handle<JS::Value> thisv, JS::Handle<JS::Value> fval, const JS::HandleValueArray & args, JS::MutableHandle<JS::Value> rval) Line 2842	C++
 	xul.dll!mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext & cx, JS::Handle<JS::Value> aThisVal, mozilla::dom::Event & event, JS::MutableHandle<JS::Value> aRetVal, mozilla::ErrorResult & aRv) Line 276	C++
 	[Inline Frame] xul.dll!mozilla::dom::EventHandlerNonNull::Call(const nsCOMPtr<mozilla::dom::EventTarget> & thisVal, mozilla::dom::Event & event, JS::MutableHandle<JS::Value> aRetVal, mozilla::ErrorResult & aRv, const char * aExecutionReason, mozilla::dom::CallbackObject::ExceptionHandling aExceptionHandling, JS::Realm * aRealm) Line 367	C++
 	xul.dll!mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event * aEvent) Line 201	C++
 	xul.dll!mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener * aListener, mozilla::dom::Event * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget) Line 1087	C++
 	xul.dll!mozilla::EventListenerManager::HandleEventInternal(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsEventStatus * aEventStatus, bool aItemInShadowTree) Line 1283	C++
 	[Inline Frame] xul.dll!mozilla::EventListenerManager::HandleEvent(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsEventStatus * aEventStatus, bool) Line 354	C++
 	xul.dll!mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor & aVisitor, mozilla::ELMCreationDetector & aCd) Line 0	C++
 	xul.dll!mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> & aChain, mozilla::EventChainPostVisitor & aVisitor, mozilla::EventDispatchingCallback * aCallback, mozilla::ELMCreationDetector & aCd) Line 559	C++
 	xul.dll!mozilla::EventDispatcher::Dispatch(nsISupports * aTarget, nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * aDOMEvent, nsEventStatus * aEventStatus, mozilla::EventDispatchingCallback * aCallback, nsTArray<mozilla::dom::EventTarget *> * aTargets) Line 1054	C++
 	xul.dll!mozilla::EventDispatcher::DispatchDOMEvent(nsISupports * aTarget, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus) Line 1163	C++
 	xul.dll!nsINode::DispatchEvent(mozilla::dom::Event & aEvent, mozilla::dom::CallerType aCallerType, mozilla::ErrorResult & aRv) Line 1302	C++
 	xul.dll!mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event & aEvent, mozilla::ErrorResult & aRv) Line 178	C++
 	xul.dll!nsContentUtils::DispatchXULCommand(nsIContent * aTarget, bool aTrusted, mozilla::dom::Event * aSourceEvent, mozilla::PresShell * aPresShell, bool aCtrl, bool aAlt, bool aShift, bool aMeta, unsigned short aInputSource) Line 5957	C++
 	xul.dll!nsXULElement::DispatchXULCommand(const mozilla::EventChainVisitor & aVisitor, nsTAutoStringN<char16_t,64> & aCommand) Line 909	C++
 	xul.dll!nsXULElement::PreHandleEvent(mozilla::EventChainVisitor & aVisitor) Line 949	C++
 	[Inline Frame] xul.dll!mozilla::EventTargetChainItem::PreHandleEvent(mozilla::EventChainVisitor & aVisitor) Line 443	C++
 	xul.dll!mozilla::EventDispatcher::Dispatch(nsISupports * aTarget, nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * aDOMEvent, nsEventStatus * aEventStatus, mozilla::EventDispatchingCallback * aCallback, nsTArray<mozilla::dom::EventTarget *> * aTargets) Line 913	C++
 	xul.dll!mozilla::EventDispatcher::DispatchDOMEvent(nsISupports * aTarget, mozilla::WidgetEvent * aEvent, mozilla::dom::Event * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus) Line 1163	C++
 	xul.dll!mozilla::PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent, mozilla::dom::Event * aEvent, nsEventStatus * aStatus) Line 8757	C++
 	xul.dll!nsContentUtils::DispatchXULCommand(nsIContent * aTarget, bool aTrusted, mozilla::dom::Event * aSourceEvent, mozilla::PresShell * aPresShell, bool aCtrl, bool aAlt, bool aShift, bool aMeta, unsigned short aInputSource) Line 5952	C++
 	xul.dll!nsXULMenuCommandEvent::Run() Line 2706	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1239	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 501	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 87	C++
 	[Inline Frame] xul.dll!MessageLoop::RunInternal() Line 315	C++
 	xul.dll!MessageLoop::RunHandler() Line 309	C++
 	xul.dll!MessageLoop::Run() Line 291	C++
 	xul.dll!nsBaseAppShell::Run() Line 139	C++
 	xul.dll!nsAppShell::Run() Line 430	C++
 	xul.dll!nsAppStartup::Run() Line 272	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4665	C++
 	xul.dll!XREMain::XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4812	C++
 	xul.dll!XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4866	C++
 	[Inline Frame] firefox.exe!do_main(int argc, char * * argv, char * * envp) Line 217	C++

Comment 1

[:Gijs (he/him)]

So my mistake here appears to be using data-l10n-id when I meant to use data-header-label-id which gets picked up by JS somewhere else.

As a result fluent tries to localize elements that have other rich content, and then apparently bad things happen (?).

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Mossop, Smaug - Should we relax the assert here and handle such scenario?

Comment 3

[Dave Townsend [:mossop]]

The problem is that we cache the prototype for an element that will be localized based on whether it has a data-l10n-id attribute during the document parse: https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/dom/prototype/PrototypeDocumentContentSink.cpp#1045.
This patch removes data-l10n-id from some elements replacing with data-header-label-id causing data-l10n-id to be set later: https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/toolkit/content/widgets/wizard.js#438.

Presumably that gets set before we do the initial translation pass so we try to cache the translation but don't have the prototype available. If we just skip then I suspect we'll ignore the mutation that sets the data-l10n-id if we think the translation is cached and so the element won't get translated correctly. If that is the case then we need to somehow get the element prototype from the element. I don't think we store that anywhere currently, doing so adds to the size of the Element class which is why I used the lookup table in the first place.

Comment 4

[:Gijs (he/him)]

Sorry, I think the initial description and patch were maybe misleading.

The final patch from bug 1645165 doesn't crash.

I can get it back to crashing by changing any of the data-header-label-id attributes to data-l10n-id, with the same value, pointing to the same fluent message. So the markup looks like this (after applying both the final patch from 1645165, and the aforementioned change):

  <wizardpage id="done" pageid="done"
              data-l10n-id="import-done-page-title">
    <description control="doneItems" data-l10n-id="import-done-description"></description>

    <vbox id="doneItems" style="overflow: auto;" align="start" role="group"/>
  </wizardpage>

with the relevant FTL:

import-done-page-title = Import Complete
import-done-description = The following items were successfully imported:

Looking at the crash in some more detail, we arrive in nsXULPrototypeDocument::RebuildL10nPrototype with aElement being the <description>, but that element is disconnected - it has a null mParent. So what I think is happening is that, in the loop in DOMLocalization::ApplyTranslations we translate the <wizardpage>, which will wipe its contents to be replaced with the Import Complete text, and then we try to translate the next element, which also works because we have our own ref to the element, but when we then try to update the proto, it doesn't work, because doc->mL10nProtoElements.Get(aElement) returns null - because when translating the parent we removed its children, which called Element::UnbindFromTree where we removed this element from the mL10nProtoElements map.

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1645246 - fluent shouldn't release-crash if one translation clobbers another element listed for translation, r?smaug

Renumbering the faux-enum while they're not (AFAICT) persisted anywhere or much used,
to ensure that we don't have to re-number items when adding different errors in the future.

In terms of the crash fix, see https://bugzilla.mozilla.org/show_bug.cgi?id=1645246#c4
for what causes it. I'm avoiding translating as well as attempting to update prototypes
for disconnected elements, which I've verified fixes the crash. I've also added a
diagnostic message so developers will still notice if this happens in future.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1645246 - add error reporting when fluent overwrites child elements (rather than just text), r?zbraniecki

Depends on D79508

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e94379891408
fluent shouldn't release-crash if one translation clobbers another element listed for translation, r=zbraniecki,smaug
https://hg.mozilla.org/integration/autoland/rev/e23a126743c3
add error reporting when fluent overwrites child elements (rather than just text), r=zbraniecki

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e94379891408
https://hg.mozilla.org/mozilla-central/rev/e23a126743c3