Closed Bug 1584674 Opened 5 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 --- wontfix
firefox70 --- unaffected
firefox71 - wontfix
firefox72 - wontfix
firefox73 - wontfix
firefox74 - wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- wontfix
firefox99 --- wontfix
firefox100 --- wontfix
firefox101 --- wontfix
firefox102 --- wontfix
firefox103 --- wontfix
firefox104 --- wontfix
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug, Regression)

Details

(5 keywords, Whiteboard: [bugmon:bisected,confirmed][post-critsmash-triage][adv-main107+r])

Attachments

(1 file, 3 obsolete files)

Attached file testcase.html (obsolete) —

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
Flags: in-testsuite?

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.

Flags: needinfo?(emilio)
Regressed by: 1553772
Keywords: regression

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

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

Keywords: sec-high

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

(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.

Flags: needinfo?(twsmith)
Attached file testcase.html (obsolete) —

More reliable test case.

Attachment #9097055 - Attachment is obsolete: true
Attached file launcher.html (obsolete) —

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.

Flags: needinfo?(twsmith)

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

Yes.

Assignee: nobody → emilio

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.

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.

Keywords: sec-highsec-moderate

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.

Has Regression Range: --- → yes
Attached file testcase.html
Attachment #9098912 - Attachment is obsolete: true
Attachment #9098914 - Attachment is obsolete: true
Severity: normal → --
Priority: P2 → --
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Severity: -- → S2
Priority: -- → P2

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.

Flags: needinfo?(emilio) → needinfo?(masayuki)

(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.

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.

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.

Flags: needinfo?(masayuki) → needinfo?(dholbert)

It seems there was a bug with the bot.

Flags: needinfo?(dholbert) → needinfo?(masayuki)

(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!

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?

Flags: needinfo?(masayuki) → needinfo?(emilio)

Yeah that seems sensible to me. Thanks!

Flags: needinfo?(emilio)

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

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)

Flags: needinfo?(masayuki)
Keywords: bugmon

(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.

Flags: needinfo?(masayuki)

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

FYI: bug 1789967 was fixed in mozilla-central.

(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.

Flags: needinfo?(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.

Flags: needinfo?(jkratzer) → needinfo?(emilio)
Keywords: bugmon
Whiteboard: [bugmon:bisected,confirmed]

Yes, that's likely given the above discussion.

Flags: needinfo?(emilio)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

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.

Group: layout-core-security → core-security-release
Depends on: 1789967
Target Milestone: --- → 107 Branch
Flags: qe-verify-
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed][post-critsmash-triage]
Whiteboard: [bugmon:bisected,confirmed][post-critsmash-triage] → [bugmon:bisected,confirmed][post-critsmash-triage][adv-main107+]
Whiteboard: [bugmon:bisected,confirmed][post-critsmash-triage][adv-main107+] → [bugmon:bisected,confirmed][post-critsmash-triage][adv-main107+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: