1517544 - A promise chain failed to handle a rejection: can't access dead object - stack: translateFragment@resource://gre/modules/DOMLocalization.jsm:680:5

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

The try push for bug 1517508 has a test failure of the following when running browser/components/sessionstore/test/browser_crashedTabs.js:

A promise chain failed to handle a rejection: can't access dead object - stack: translateFragment@resource://gre/modules/DOMLocalization.jsm:680:5 translateRoots/<@resource://gre/modules/DOMLocalization.jsm:581:15 Async*translateRoots@resource://gre/modules/DOMLocalization.jsm:575:7 onChange@resource://gre/modules/DOMLocalization.jsm:436:5 removeResourceIds@resource://gre/modules/Localization.jsm:165:5 Rejection date: Thu Jan 03 2019 17:11:20 GMT+0000 (Coordinated Universal Time) - false == true

JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1131
chrome://mochikit/content/browser-test.js:Tester_execTest:1097
chrome://mochikit/content/browser-test.js:nextTest/<:995
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1517544 - Use a WeakSet to store references to the localization roots so we don't try updating them after GC.

Comment 2

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

> The try push for bug 1517508 has a test failure of the following when running browser/components/sessionstore/test/browser_crashedTabs.js:

why is `removeResourceIds` called here?

Is it manually removed, or is it due to the document being destroyed? If the latter, can we instead/also improve the logic in [0] to not fire it at all if the removal happens due to document being shutdown?

[0] https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#3210

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I can reproduce this by refreshing the about:robots page multiple times while attached to my debugger.

This is the callstack:
> xul.dll!nsIDocument::LocalizationLinkRemoved(mozilla::dom::Element * aLinkElement) Line 3209  C++
> xul.dll!mozilla::dom::HTMLLinkElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 172  C++
> xul.dll!mozilla::dom::Element::UnbindFromTree(bool aDeep, bool aNullParent) Line 0  C++
> xul.dll!nsGenericHTMLElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 434 C++
> xul.dll!mozilla::dom::Element::UnbindFromTree(bool aDeep, bool aNullParent) Line 0  C++
> xul.dll!nsGenericHTMLElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 434 C++
> xul.dll!mozilla::dom::HTMLSharedElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 261  C++
> xul.dll!mozilla::dom::Element::UnbindFromTree(bool aDeep, bool aNullParent) Line 0  C++
> xul.dll!nsGenericHTMLElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 434 C++
> xul.dll!mozilla::dom::HTMLSharedElement::UnbindFromTree(bool aDeep, bool aNullParent) Line 261  C++
> xul.dll!nsIDocument::cycleCollection::Unlink(void * p) Line 1862  C++
> xul.dll!nsHTMLDocument::cycleCollection::Unlink(void * p) Line 187  C++
> xul.dll!nsCycleCollector::CollectWhite() Line 3086  C++
> xul.dll!nsCycleCollector::Collect(ccType aCCType, js::SliceBudget & aBudget, nsICycleCollectorListener * aManualListener, bool aPreferShorterSlices) Line 3430  C++
> xul.dll!nsCycleCollector_collectSlice(js::SliceBudget & budget, bool aPreferShorterSlices) Line 3956  C++
> xul.dll!nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp aDeadline) Line 1473 C++
> xul.dll!ICCRunnerFired(mozilla::TimeStamp aDeadline) Line 1519  C++
> [External Code] 
> xul.dll!mozilla::IdleTaskRunner::Run() Line 63  C++
> xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1144 C++
> xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 468  C++
> xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 88 C++
> xul.dll!MessageLoop::RunHandler() Line 308  C++
> xul.dll!MessageLoop::Run() Line 290 C++
> xul.dll!nsBaseAppShell::Run() Line 139  C++
> xul.dll!nsAppShell::Run() Line 409  C++
> xul.dll!nsAppStartup::Run() Line 272  C++
> xul.dll!XREMain::XRE_mainRun() Line 4616  C++
> xul.dll!XREMain::XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4754  C++
> xul.dll!XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4839 C++
> firefox.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 293 C++
> firefox.exe!wmain(int argc, wchar_t * * argv) Line 129  C++

We shouldn't call LocalizationLinkRemoved in HTMLLinkElement::UnbindFromTree if the ownerDoc or the global is null, but I don't see other code in dom/html that needs to do similar checks.

@Andrew, is there a need to run UnbindFromTree during cycle collection or could there be a faster path? If we do need to follow this code path, should we check if the owner document is still around before calling LocalizationLinkRemoved?

Comment 4

[Andrew McCreight [:mccr8]]

Hmm I'm not really sure unfortunately. Olli might know.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I got help from Olli over IRC.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> > The try push for bug 1517508 has a test failure of the following when running browser/components/sessionstore/test/browser_crashedTabs.js:
> 
> why is `removeResourceIds` called here?
> 
> Is it manually removed, or is it due to the document being destroyed? If the
> latter, can we instead/also improve the logic in [0] to not fire it at all
> if the removal happens due to document being shutdown?
> 
> [0] https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#3210

It is being called from HTMLLinkElement::UnbindFromTree during cycle collection. I wrote a patch that tried to check if the global was still alive but that didn't fix the error. I believe the global was being kept alive from DOMLocalization.jsm's this.roots set, which was keeping a strong reference to the elements. The attached patch moves this from a Set to a WeakSet. The test failure no longer occurs now.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

FYI, this is waiting on bug 1518252

Comment 8

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

With bug 1518252 fixed, I'm trying to reproduce this bug and can't, potentially not because it doesn't happen, but because this test crashes for me earlier on:

browser/components/sessionstore/test/browser_crashedTabs.js
  FAIL Uncaught exception - Content has URI about:blank which does not match data:text/html,<html><body>A%20regular,%20everyday,%20normal%20page.
  FAIL Should have closed the tab - Got 2, expected 1
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1318
chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_crashedTabs.js:test_close_tab_after_crash:370
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1108
chrome://mochikit/content/browser-test.js:Tester_execTest:1099
chrome://mochikit/content/browser-test.js:nextTest/<:997
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
  FAIL Found an unexpected tab at the end of test run: data:text/html,<html><body>Another%20regular,%20everyday,%20normal%20page. -
  FAIL Found an unexpected browser window at the end of test run -

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1517544 - Don't update the translations if there are no Fluent resources left.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5dd9eaf04f35
Don't update the translations if there are no Fluent resources left. r=zbraniecki

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5dd9eaf04f35