1584674 - Assertion failure: !mForbiddenToFlush (This is bad!), at src/layout/base/PresShell.cpp:3966

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found with m-c 20190927-dcfdecc355c0

Setting s-s flag based on the assertion message. Feel free to ping me if this is incorrect.

Assertion failure: !mForbiddenToFlush (This is bad!), at src/layout/base/PresShell.cpp:3966

#0 RecordMeasurementEnd src/obj-firefox/dist/include/mozilla/PerfStats.h:47:11
#1 ~AutoMetricRecording src/obj-firefox/dist/include/mozilla/PerfStats.h:57
#2 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4142
#3 FlushPendingNotifications src/obj-firefox/dist/include/mozilla/PresShell.h:1451:5
#4 mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10073
#5 nsEditingSession::SetupEditorOnWindow(nsPIDOMWindowOuter&) src/editor/composer/nsEditingSession.cpp:299:10
#6 nsEditingSession::MakeWindowEditable(mozIDOMWindowProxy*, char const*, bool, bool, bool) src/editor/composer/nsEditingSession.cpp:166:10
#7 mozilla::dom::Document::EditingStateChanged() src/dom/base/Document.cpp:5279:25
#8 mozilla::dom::Document::EndUpdate() src/dom/base/Document.cpp:7074:3
#9 ~mozAutoDocUpdate src/dom/base/mozAutoDocUpdate.h:34:18
#10 mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/dom/base/Element.cpp:2386
#11 SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:835:12
#12 SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:831
#13 mozilla::ScrollFrameHelper::SetCoordAttribute(mozilla::dom::Element*, nsAtom*, int) src/layout/generic/nsGfxScrollFrame.cpp:6322
#14 mozilla::ScrollFrameHelper::UpdateScrollbarPosition() src/layout/generic/nsGfxScrollFrame.cpp:5060:5
#15 mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsAtom*) src/layout/generic/nsGfxScrollFrame.cpp:3010:5
#16 mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, nsAtom*) src/layout/generic/nsGfxScrollFrame.cpp:2210:3
#17 mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) src/layout/generic/nsGfxScrollFrame.cpp:2342:5
#18 mozilla::ScrollFrameHelper::ScrollTo(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) src/layout/generic/nsGfxScrollFrame.cpp:2245:3
#19 mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() src/layout/generic/ScrollAnchorContainer.cpp:378:17
#20 FlushPendingScrollAnchorAdjustments src/layout/base/PresShell.cpp:2566:23
#21 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4162
#22 mozilla::PresShell::DidDoReflow(bool) src/layout/base/PresShell.cpp:9035:3
#23 mozilla::PresShell::ProcessReflowCommands(bool) src/layout/base/PresShell.cpp:9408:7
#24 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4159:11
#25 FlushPendingNotifications src/obj-firefox/dist/include/mozilla/PresShell.h:1451:5
#26 nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2011
#27 TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
#28 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350
#29 mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
#30 RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:807:5
#31 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:727
#32 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) src/layout/base/nsRefreshDriver.cpp:622:9
#33 mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) src/layout/ipc/VsyncChild.cpp:65:16
#34 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:187:54
#35 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
#36 mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2185:25
#37 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2109:9
#38 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1954:3
#39 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1985:13
#40 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
#41 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
#42 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
#43 RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
#44 RunHandler src/ipc/chromium/src/base/message_loop.cc:308
#45 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290
#46 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#47 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
#48 RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
#49 RunHandler src/ipc/chromium/src/base/message_loop.cc:308
#50 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290
#51 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
#52 content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#53 main src/browser/app/nsBrowserApp.cpp:272

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Fun! I guess this is caused by the assertions I added in bug 1553772.

So not great, though pre-existing, as this is me trying to harden our code against reentrant layouts like these, which are quite nasty.

Comment 2

[Daniel Veditz [:dveditz]]

"caused" by the assertions, or merely detected by them?

Assuming assertions that say "this is bad" mean it: sec-high

Comment 3

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/xZpaTpbBYNlOZVl8rCDF6w/index.html

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Veditz [:dveditz] from comment #2)

"caused" by the assertions, or merely detected by them?

Detected by, of course.

(In reply to Tyson Smith [:tsmith] from comment #3)

A Pernosco session is available here: https://pernos.co/debug/xZpaTpbBYNlOZVl8rCDF6w/index.html

Awesome! Tyson, could you attach the test-case you reproduced this with? I see that it's calling a few different functions, so the test-case is a bit different.

For example in the pernosco session the entry point is scrollContentIntoView() called from RAF callbacks, the attached test-case has no such call.

Comment 5

[Tyson Smith [:tsmith]]

Attachment: testcase.html

More reliable test case.

Comment 6

[Tyson Smith [:tsmith]]

Attachment: launcher.html

emilio: Let me know if testcase.html has what you expect and if not let me know if you'd like a new Pernosco session.

Comment 7

[Sean Voisen (:svoisen)]

Emilio: Are you actively working on this? Can we assign to you?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yes.

Comment 9

[Pascal Chevrel:pascalc]

Unlikely to be fixed in 71 given where we are in the schedule, I am keeping it as fix-optional for 71 because it is as sec-high issue, in case we have a late dot release and a fix happens.

Comment 10

[Julien Cristau [:jcristau]]

Any progress here?

Comment 11

[Sean Voisen (:svoisen)]

Emilio's been away on PTO. Though from conversations with Emilio I think we can safely say this is not sec-high. The assertions were recently added, and the re-entrant layout was detected by the new assertions, not caused by them.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I don't think this needs to be tracked if it's a sec-moderate, but I'd still take a low-risk fix in Fx73 if one were available.

Comment 13

[Tyson Smith [:tsmith]]

A new Pernosco session is available here: https://pernos.co/debug/LN7etsHmxFDZxY0OWQvvFQ/index.html

Comment 14

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Okay, so I took a look at this. The issue is that editor fails to initialize because this fails, and we semi-randomly try to initialize on the next Document::EndUpdate, which is a bad time.

Masayuki, when the editor fails to initialize, can we either avoid trying to initialize again, or make the retry more deterministic? It seems bad to potentially be in a state where every document update tries to initialize the editor...

For reference, editor failing to initialize here.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Masayuki, when the editor fails to initialize, can we either avoid trying to initialize again, or make the retry more deterministic? It seems bad to potentially be in a state where every document update tries to initialize the editor...

In general speaking, it's hard to say. I have no idea of general solution. However, in this case, it seems that HTMLEditor should ignore the selection initialization failure because it can be valid, and HTMLEditor should not assume that Selection::RangeCount returns non-zero in the initialization path. But I've not checked enough yet. I'll be back later.

Comment 18

[Armin Ebert [:arminius]]

Just FYI, there's an additional testcase in my duplicate bug 1774383. (See attachment 9281391 [details])

AFAIU it arises from the same root issue (i.e. editor initialization), but gets there via an XML document.

Comment 19

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

Redirect a needinfo that is pending on an inactive user to the triage owner.
:dholbert, since the bug has high priority, high severity and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

It seems there was a bug with the bot.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away -8/31) from comment #16)

But I've not checked enough yet. I'll be back later.

Hi Masayuki - just checking in to be sure this is still on your radar. Thanks!

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

EditorBase::CollapseSelectionToEndOf is called only in the following stack for initializing the HTMLEditor:

1. EditorBase::InitEditorContentAndSelection
2. HTMLEditor::InitEditorContentAndSelection
3. HTMLEditor::Init

There is bug 195802... So it might have already been caused infinite retrying, though. Perhaps, the ideal fix is, HTMLEditor::Init should return special error code to make the caller retry it later. However, fixing this bug for now, I think that we should make EditorBase::InitEditorContentAndSelection be careful more before calling CollapseSelectionToEndOf.

Should I fix the editor part in a public bug?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Yeah that seems sensible to me. Thanks!

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Filed bug 1789967 for fixing the editor's initialization bug (but not linking it with this bug due to security reason).

Comment 25

[Daniel Holbert [:dholbert]]

Looks like we have patches on bug 1789967 now, which is great! Just checking, have you confirmed that they seem to fix this issue? (if you can repro with one of the testcases)

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Holbert [:dholbert][mostly away until Oct 6] from comment #25)

Looks like we have patches on bug 1789967 now, which is great! Just checking, have you confirmed that they seem to fix this issue? (if you can repro with one of the testcases)

I ran the testcases for a while in debug build, but I don't see any crashes/assertion hits.

Comment 27

[Jason Kratzer [:jkratzer]]

I am still able to reproduce this issue on mozilla-central 20220920-45d33d6757ba.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: bug 1789967 was fixed in mozilla-central.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Jason Kratzer [:jkratzer] from comment #27)

I am still able to reproduce this issue on mozilla-central 20220920-45d33d6757ba.

Could you post a pernosco trace? Sounds like it might be fiddly / difficult to repro+investigate without a trace, per comment 26.

Comment 30

[Jason Kratzer [:jkratzer]]

Bugmon Analysis
Testcase crashes using the initial build (mozilla-central 20220701213554-5140fba12e4a) but not with tip (mozilla-central 20221011093208-5cbd3d92a78c.)

The bug appears to have been fixed in the following build range:

Start: 4d1e93c629daa361f9acd60c1f4e2c594f3bd312 (20220922062700)
End: 4bfcb4ab080ffa3b69f831c840d1212e94fc7199 (20220922063009)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4d1e93c629daa361f9acd60c1f4e2c594f3bd312&tochange=4bfcb4ab080ffa3b69f831c840d1212e94fc7199

emilio, can you confirm that the above bisection range is responsible for fixing this issue?
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Yes, that's likely given the above discussion.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, but bug 1789967 did not fix the root cause (infinite retries of initializing HTMLEditor), so I guess that there is another way to reproduce this.