1530177 - Crash in [@ mozilla::PresShell::DoFlushPendingNotifications] with MOZ_RELEASE_ASSERT(!mForbiddenToFlush)

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-c176ac7d-84b8-474f-9671-aa29d0190224.

Top 10 frames of crashing thread:

0 xul.dll mozilla::PresShell::DoFlushPendingNotifications layout/base/PresShell.cpp:4017
1 xul.dll mozilla::dom::Document::FlushPendingNotifications dom/base/Document.cpp:7065
2 xul.dll class mozilla::dom::Element* nsFocusManager::CheckIfFocusable dom/base/nsFocusManager.cpp:1482
3 xul.dll void nsFocusManager::Focus dom/base/nsFocusManager.cpp:1820
4 xul.dll nsFocusManager::WindowRaised dom/base/nsFocusManager.cpp:691
5 xul.dll void nsWebShellWindow::WindowActivated xpfe/appshell/nsWebShellWindow.cpp:456
6 xul.dll nsWebShellWindow::WidgetListenerDelegate::WindowActivated xpfe/appshell/nsWebShellWindow.cpp:805
7 xul.dll nsWindow::ProcessMessage widget/windows/nsWindow.cpp:5712
8 xul.dll static long nsWindow::WindowProcInternal widget/windows/nsWindow.cpp:4753
9 xul.dll CallWindowProcCrashProtected xpcom/base/nsCrashOnException.cpp:27

crash reports with this signature and MOZ_RELEASE_ASSERT(!mForbiddenToFlush) (This is bad!) are starting to appear in 67.0a1 and 66.0b10.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I cannot see other reports:

An unexpected error occured :(
We have been automatically informed of that error, and are working on a solution.
error - Forbidden

If you can see them, do all of them have nsImageBoxFrame on the stack? Or is there something else interesting? I guess I can remove this assertion for nsImageBoxFrame for now, though it is really bad. It's the same root cause as bug 1438939.

Comment 2

[[:philipp]]

this query should list more reports: https://bit.ly/2BP21IY
i checked and only a quarter of them had nsImageBoxFrame in their stack...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1530177 - Downgrade an assertion to a diagnostic assert since it exposes pre-existing bugs.

nsIconChannel (for moz-icon:// images) is unsound, see bug 1438939.

nsMenuPopupFrame::Init is also unsound on mac, looks like...

I'll try to get them fixed on trunk, but it's not worth crashing release for
this IMO, given it's pre-existing. The assert in PresShell::~PresShell hopefully
avoids exploitable issues.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I want to avoid crashing but leave this open until the dependent issues are fixed.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9046206 [details]
Bug 1530177 - Downgrade an assertion to a diagnostic assert since it exposes pre-existing bugs.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1525509
• User impact if declined: Crashes.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): Just makes an assertion introduced in bug 1525509 not crash on release, since fixing the dependent pre-existing issues it exposed will take a bit.
• String changes made/needed: none

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9046206 [details]
Bug 1530177 - Downgrade an assertion to a diagnostic assert since it exposes pre-existing bugs.

Thanks, let's uplift for beta 11.

Comment 7

[[:philipp]]

MOZ_RELEASE_ASSERT(!mForbiddenToFlush)(This is bad!) got introduced in 67.0a1 and uplifted to 66.0b10 - so this particular variant of the [@ mozilla::PresShell::DoFlushPendingNotifications] signature the bug was filed for doesn't affect prior versions...

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/abafbd8bbe09

Comment 9

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

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See How Do You Triage for more information

Comment 10

[Sean Voisen (:svoisen)]

Assigning to Emilio since he already fixed it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bb743ffae8c
Downgrade an assertion to a diagnostic assert since it exposes pre-existing bugs. r=dholbert

Comment 12

[Daniel Varga [:dvarga]]

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

Comment 13

[Neha Kochar [:neha]]

Daniel, there is a commit in central (comment 12) so this should be marked resolved fixed, correct?

Comment 14

[Daniel Varga [:dvarga]]

Hi Neha, this bug has leave-open, that's why it wasn't marked as resolved fixed.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Yeah, this is not crashing release, but dependent bugs need to be fixed for this to be properly fixed.

Comment 16

[Neha Kochar [:neha]]

Marking it disabled for 67 as the MOZ_RELEASE_ASSERT has been downgraded to a diagnostic assert in 67.

Comment 17

[Marcia Knous [:marcia]]

Hello Emilio - the dependent bugs listed are now fixed, but I still see crashes in 69 and 70 nightly. Most of the ones in 70 (https://mzl.la/2MeHoNt) have MOZ_DIAGNOSTIC_ASSERT(!mDocument->GetPresShell()) (Where did this shell come from?) as the moz crash reason. Is this a different issue?

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Yeah, that looks like a different issue. I filed bug 1573823 for that.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Well, maybe we should file a new bug for the !mDocument->GetPresShell() assertion and close this one as fixed, your call Marcia.

Comment 20

[Marcia Knous [:marcia]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)

Well, maybe we should file a new bug for the !mDocument->GetPresShell() assertion and close this one as fixed, your call Marcia.

I think we can keep this open until the dependent bug in Comment 18 is addressed.

Comment 21

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

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

no

Comment 23

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

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Dependencies are fixed, new crashes should probably get new bugs.