1751836 - Crash in [@ mozilla::dom::Promise::AppendNativeHandler]

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect)

Description

[Asif Youssuff]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/c06b7eaf-07d6-41b4-9533-3fbb70220125

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so mozilla::dom::Promise::AppendNativeHandler dom/promise/Promise.cpp:423
1 libxul.so mozilla::dom::L10nMutations::FlushPendingTranslations dom/l10n/L10nMutations.cpp:221
2 libxul.so nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:2338
3 libxul.so mozilla::RefreshDriverTimer::TickRefreshDrivers layout/base/nsRefreshDriver.cpp:326
4 libxul.so mozilla::RefreshDriverTimer::Tick layout/base/nsRefreshDriver.cpp:342
5 libxul.so mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver layout/base/nsRefreshDriver.cpp:703
6 libxul.so mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyParentProcessVsync layout/base/nsRefreshDriver.cpp:620
7 libxul.so mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run layout/base/nsRefreshDriver.cpp:510
8 libxul.so mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:770
9 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1195

Not sure this is the right component - picked graphics based on the trace showing Vsync stuff.

Other crashes from the same profile:

bp-2674a529-f673-48d6-8ba7-0f7aa0220125
bp-d48032a8-d755-4920-854d-378e60220125
bp-cf2a77b7-430d-47d4-8b8c-395070220125
bp-a986d161-4760-4299-b06d-c9c770220125

Firefox crashes while doing a session restore.

Comment 1

[Asif Youssuff]

Decided to try a mozregression with just the sessionstore file and using the History > Restore previous session feature (in a blank profile).

Oddly enough, came up with:

25:10.26 INFO: Narrowed integration regression window from [6af6b19b, e0d60e01] (4 builds) to [286aa538, e0d60e01] (2 builds) (~1 steps left)
25:10.26 INFO: No more integration revisions, bisection finished.
25:10.26 INFO: Last good revision: 286aa538453c96873c4093a13662a9cae6b35a37
25:10.26 INFO: First bad revision: e0d60e01952768100a564dd1b6a39bdc2de323c2
25:10.26 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=286aa538453c96873c4093a13662a9cae6b35a37&tochange=e0d60e01952768100a564dd1b6a39bdc2de323c2

bug 1739143

No idea why this is causing this issue, but I ran the mozregression twice and it came up both times.

Comment 2

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

Set release status flags based on info from the regressing bug 1739143

Comment 3

[Christian Holler (:decoder)]

Erik, I think the mozregression info is right here, can you look into this?

Comment 4

[Erik Nordin [:nordzilla]]

Thanks for the NI. I am leaving it open for now. I hope to have time to look at this next week.

Comment 5

[Pascal Chevrel:pascalc]

Too late for a fix in 98

Comment 6

[Erik Nordin [:nordzilla]]

I am trying to reproduce this but the Restore previous session feature seems to be working for me using a clean profile on the latest nightly build.

100.0a1 20220325093814

Can you verify for me whether this is still affecting you, and if so, give me a clear set of steps to reproduce?

Comment 7

[Asif Youssuff]

Erik, I can reproduce the issue and it is really simple - I just try to restore the broken sessionstore file. I'd rather not share the sessionstore file, so if there is a special build or some kind of logging you want me to perform, I am happy to do it. Or if there is a simple way to sanitize the file, I suppose I could try that to see if the issue reproduces.

Comment 8

[Erik Nordin [:nordzilla]]

I'd rather not share the sessionstore file

I totally understand! I don't expect you to have to share any personal information.

Unfortunately, I am still unable to reproduce the crash myself, on Ubuntu 20.

Is the crash happening only with your particular session restore file, or is this happening for all session restores?

If the situation is the latter, are you able to reproduce the crash with a different session in a clean profile?

Comment 9

[Asif Youssuff]

Is the crash happening only with your particular session restore file, or is this happening for all session restores?

A particular session restore file. :/

Comment 10

[Dan Minor [:dminor]]

This is crashing in [1] which was added in bug 1739143, when we try to access the promise created at [2]. Looking at TranslateElements [3], there are a lot of code paths that return nullptr. I think this just needs a nullptr check before we try to use the promise.

[1] https://searchfox.org/mozilla-central/rev/711e1cea1cb584057c50aac0a26a3f7c969eda66/dom/l10n/L10nMutations.cpp#221
[2] https://searchfox.org/mozilla-central/rev/711e1cea1cb584057c50aac0a26a3f7c969eda66/dom/l10n/L10nMutations.cpp#217.
[3] https://searchfox.org/mozilla-central/rev/711e1cea1cb584057c50aac0a26a3f7c969eda66/dom/l10n/DOMLocalization.cpp#302

Comment 11

[Erik Nordin [:nordzilla]]

Hi Asif,

Based on Dan's insight above, I landed a patch to fix the erroneous code. We believe that it may be a fix for the issue you are encountering as well.

Can you try your method of reproducing the behavior in the most current Nightly build and let me know if the behavior is resolved?

Thanks!

Comment 12

[Asif Youssuff]

Erik,

I managed to crash it again, unfortunately. See crash ids:

bp-f8327117-2c6d-40e6-9e07-f06e40220404
bp-fd265237-c6af-4a3c-ba2c-4c0330220404

Thanks,
Asif

Comment 13

[Erik Nordin [:nordzilla]]

I do believe that the error that Dan pointed out was legitimate, and I am glad that it is now fixed.

Unfortunately, it does not seem to have resolved the entirety of this behavior that we are seeing.

Based on these new crash reports, I do not see any Internationalization- or Localization-related code in the stack trace.

This appears to be hitting the same path as bug 1724000

I have added this bug to the "see also" list of bug 1724000, and for now I am going to close this bug as a duplicate in favor of bug 1724000, since the crash appears to be due to the same reason and because bug 1724000 has existed for a longer period of time.

If you have any reason to believe that this bug should be reopened individually, please feel free to do so at any time!

Thank you for reporting this, and for your prompt responses!

Comment 15

[:Gijs (he/him)]

(In reply to Erik Nordin [:nordzilla] from comment #11)

Hi Asif,

Based on Dan's insight above, I landed a patch to fix the erroneous code. We believe that it may be a fix for the issue you are encountering as well.

Can you try your method of reproducing the behavior in the most current Nightly build and let me know if the behavior is resolved?

Thanks!

In future, can you please link to the relevant bug (ie where the patch landed) and dupe there, as that was what fixed the originally reported crash, with the "new"/revealed crash being bug 1724000? At the moment it isn't clear where that happened, though the crash stats info suggests Fx 100 will fix it.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I think the code here is still wrong.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751836 - Check for errors properly in DOMLocalization::TranslateRoots. r=nordzilla,Gijs

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751836 - Ignore error properly in L10nMutations::FlushPendingTranslations(). r=nordzilla,Gijs

If we're dropping the ErrorResult on the floor we should use
IgnoredErrorResult.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Bug 1762476 is what fixed this particular crash per se, probably. But relevant code is still not correct.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751836 - Drive-by: Simplify L10nMutations::FlushPendingTranslations. r=Gijs,nordzilla

Sequence can be downcasted to a const nsTArray, so we can clean up the
code a bit.

Depends on D145188

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8d43ccb7e3a
Check for errors properly in DOMLocalization::TranslateRoots. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/587daff80d9a
Ignore error properly in L10nMutations::FlushPendingTranslations(). r=Gijs

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c8d43ccb7e3a
https://hg.mozilla.org/mozilla-central/rev/587daff80d9a

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/440ff4389328
Drive-by: Simplify L10nMutations::FlushPendingTranslations. r=dminor

Comment 24

[Erik Nordin [:nordzilla]]

(In reply to :Gijs (he/him) from comment #15)

(In reply to Erik Nordin [:nordzilla] from comment #11)

Hi Asif,

Based on Dan's insight above, I landed a patch to fix the erroneous code. We believe that it may be a fix for the issue you are encountering as well.

Can you try your method of reproducing the behavior in the most current Nightly build and let me know if the behavior is resolved?

Thanks!

In future, can you please link to the relevant bug (ie where the patch landed) and dupe there, as that was what fixed the originally reported crash, with the "new"/revealed crash being bug 1724000? At the moment it isn't clear where that happened, though the crash stats info suggests Fx 100 will fix it.

Of course, I'm sorry to have caused confusion here.

Comment 25

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/440ff4389328