2010601 - Crash in [@ stackoverflow | nsIFrame::GetOffsetToCrossDoc]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

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

[Tracking Requested - why for this release]:
Crash signature new in Firefox 147.0, all reports for Windows. 38 crash reports from 37 installs.

The top of the stack is nested rendering which originates from

...
mozilla::PresShell::PaintInternal(nsIFrame*, mozilla::WindowRenderer*, mozilla::PaintInternalFlags)	layout/base/PresShell.cpp:6573
mozilla::PresShell::PaintAndRequestComposite(nsIFrame*, mozilla::WindowRenderer*, mozilla::PaintFlags)	layout/base/PresShell.cpp:6435
mozilla::PresShell::PaintSynchronously()	layout/base/PresShell.cpp:12566
mozilla::PresShellWidgetListener::PaintWindow(nsIWidget*)	layout/base/PresShellWidgetListener.cpp:210
mozilla::widget::PuppetWidget::Paint()	widget/PuppetWidget.cpp:892
mozilla::widget::PuppetWidget::WidgetPaintTask::Run()	widget/PuppetWidget.cpp:882
mozilla::RunnableTask::Run()	xpcom/threads/TaskController.cpp:705 

Crash report: https://crash-stats.mozilla.org/report/index/ffa8c3ea-861a-4336-85f4-b39cb0260115

Reason:

EXCEPTION_STACK_OVERFLOW

Top 10 frames:

0  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
1  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
2  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
3  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
4  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
5  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
6  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
7  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
8  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982
9  xul.dll  nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const  layout/generic/nsIFrame.cpp:7982

Comment 1

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

Could you check what this is about? Recent changes in the PuppetWidget code.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

That stack doesn't say much, PuppetWidget is a bit of a red herring. I think this is most likely from bug 1994942.

Unfortunately there are no urls on the crashes... Jason, have fuzzers seen this by any chance?

(Also a bit weird that all the crashes are on windows...)

Comment 3

[Daniel Holbert [:dholbert]]

There are two crashes from beta that have a URL (same URL, & same install, it looks like):
https://crash-stats.mozilla.org/report/index/f76ce5ed-f478-4e8f-94bc-9f7840251230
https://crash-stats.mozilla.org/report/index/faba46d5-0fcb-463b-973c-b24bb0251230

The page there does have two links that open an in-content modal element with an <object> pointing at a PDF, which might be how GetOffsetToCrossDoc comes into play here. Having said that, I can't repro the crash here (tested in Nightly and release on linux, and 147beta on Windows 11 via BrowserStack)

Comment 4

[Daniel Holbert [:dholbert]]

Looking at the backtrace in the crash report, nsIFrame::GetOffsetToCrossDoc(nsIFrame const*, const int) const is making a recursive call (to itself) in this section:
https://searchfox.org/firefox-main/rev/60a466f9e65cc72cd0f57a26a109b63a2c4d9c00/layout/generic/nsIFrame.cpp#7991-7997

// Looks like aOther wasn't an ancestor of |this|.  So now we have
// the root-document-relative position of |this| in |offset|. Subtract the
// root-document-relative position of |aOther| from |offset|.
// This call won't try to recurse again because root is an ancestor of
// aOther.
nsPoint negOffset = aOther->GetOffsetToCrossDoc(root, aAPD);
offset -= negOffset;

Note the code-comment promises that we won't go any further into a recursive-death-spiral -- "This call won't try to recurse again because root is an ancestor of aOther" -- but apparent that promise is misguided in some way.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Yeah I suspect what's going on is that somehow we fail to sever the relationship between the frame and the parent document in one direction but not the other or so...

Comment 6

[Daniel Holbert [:dholbert]]

Some quick notes from code inspection. [was going to write a bit more but I just found STR so I'm posting this now and I'll post the STR in the next comment)

We get into GetOffsetToCrossDoc from this call in nsSubDocumentFrame::BuildDisplayList:
https://searchfox.org/firefox-main/rev/60a466f9e65cc72cd0f57a26a109b63a2c4d9c00/layout/generic/nsSubDocumentFrame.cpp#372-373,393,403,405

RefPtr<mozilla::PresShell> presShell = GetSubdocumentPresShellForPainting(
    aBuilder->IsIgnoringPaintSuppression() ? IGNORE_PAINT_SUPPRESSION : 0);
...
nsIFrame* subdocRootFrame = presShell->GetRootFrame();
...
if (subdocRootFrame) {
...
  visible = aBuilder->GetVisibleRect() + GetOffsetToCrossDoc(subdocRootFrame);

Note that GetSubdocumentPresShellForPainting has variable behavior depending on whether we're printing, which might turn out to be important...

When we enter the outermost GetOffsetToCrossDoc call...

• this is the nsSubDocumentFrame
• aOther is the root of its subdocument
• So, the first line that I quoted in comment 4 (aOther wasn't an ancestor of |this|) is accurate. (They belong to different documents, and aOther is nested inside of the subdocument that this owns/represents.)
• It looks like root will end up being the root of the outermost in-process document (it gets set by walking up the chain of ancestors from this, including tunneling through same-process iframe container boundaries via GetCrossDocParentFrameInProcess).
• So when we make the first recursive call...
  https://searchfox.org/firefox-main/rev/60a466f9e65cc72cd0f57a26a109b63a2c4d9c00/layout/generic/nsIFrame.cpp#7994-7996

// This call won't try to recurse again because root is an ancestor of
// aOther.
nsPoint negOffset = aOther->GetOffsetToCrossDoc(root, aAPD);

...I think it is true that root is an ancestor of aOther (using ancestor loosely, including GetCrossDocParentFrameInProcess traversals).

Comment 7

[Daniel Holbert [:dholbert]]

STR to trigger this bug:
(1) Visit the URL from the comment 3 crash reports.
(2) Click the link with a picture of a little book, which brings up an embedded PDF view. (Note, the embedded PDF is super small in Firefox; I've got a local testcase for that which I'll reduce & file separately.)
(3) Ctrl+P to open print preview.

Here's my Nightly crash report:
bp-67a310ca-87d7-488d-b9ea-74cf30260115

Not stack exhaustion, but a diagnostic assert (which is probably a prelude to what would otherwise become a stack exhaustion):

MOZ_DIAGNOSTIC_ASSERT(PresContext()->GetRootPresContext() == aOther->PresContext()->GetRootPresContext()) (trying to get the offset between frames in different document hierarchies?)

Comment 8

[Daniel Holbert [:dholbert]]

I'll have a reduced testcase to post shortly.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: simple PDF, for use in testcase

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (crashes when print-previewed)

Comment 11

[Daniel Holbert [:dholbert]]

With that testcase, I confirmed that this was a regression from bug 2010601. There was an intermediate bad behavior where we'd hang instead of crashing when I print-preview):

First regression range (going from "good" behavior to hanging-during-print-preview):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a3f29f0c5c882ca9667bcc4e08350d16b85be4ca&tochange=6327748c3d3d9f87da9445c71014408cc4026382
--> behavior-change caused by https://hg-edge.mozilla.org/integration/autoland/rev/6327748c3d3d9f87da9445c71014408cc4026382

Second regression range (going from hanging-during-print-preview to crashing-during-print-preview:
--> behavior-change caused by https://hg-edge.mozilla.org/integration/autoland/rev/83ed52ad40f5462dc6eaf891c449f2f70ff52bb0 (presumably just fixing the hang, which in this case means we proceed to crash instead)

Comment 12

[Daniel Holbert [:dholbert]]

Tentatively triaging as S2 given relatively low crash volume. Probably we should fix ASAP and try to get a fix into a dot release, though.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)

Tentatively triaging as S2 given relatively low crash volume

Er sorry, for "relatively low crash volume" I was looking at the graph which currently shows no more than 5 crashes per day. But that's just because (for now) that visualization ends on Jan 13 which is where this picked up (when 147 went to release); and since then we've had many more crashes.

Let's call this S1.

Comment 14

[Daniel Holbert [:dholbert]]

In a debug build, print-previewing testcase 1 gives this fatal-assertion (which presumably we traverse before the diagnostic assertion that we hit in opt builds):
Assertion failure: IsHTMLElement(nsGkAtoms::area) || !aFrame || !mPrimaryFrame || aFrame == mPrimaryFrame (Losing track of existing primary frame), at /builds/worker/checkouts/gecko/dom/base/nsIContentInlines.h:33'

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: backtrace of "Losing track of existing primary frame" assertion-failure in local debug+opt build

Here's the backtrace of that debug-build assertion-failure.

(I'll stop poking here since it looks like emilio's on it - thanks emilio!)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Per Slack discussion with Daniel, let's leave this as S2.

Comment 17

[Daniel Holbert [:dholbert]]

In case emilio hasn't already done so & it turns out to be useful for debugging/fixing... I've captured an rr recording of this bug (when print-previewing the attached "testcase 1"), in a debug build where I modified both aforementioned assertions (comment 7 & comment 14) to be non-fatal NS_ASSERTION so that I could proceed all the way to the stack-exhaustion crash. Submitting that to pernosco now; I'll share the link when I've got it.

Comment 18

[Daniel Holbert [:dholbert]]

Pernosco trace:
https://pernos.co/debug/fxzuXz0j8DNG1WEWMsp8ZQ/index.html#f{m[DMav,AQ_,t[/Q,BvlF_,f{e[DMaq,R0rA_,s{adItNAAAA,bEx8,uAT8bEw,oAUF0KQ___/

This is seeked to the first "Losing track of existing primary frame" assertion-failure (which, as noted in comment 17, I've locally made into a non-fatal NS_ASSERTION so that it's possible to proceed further if desired).

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2010601 - Avoid losing track of primary frame in nsSubDocumentFrame::Init. r=dholbert,jfkthame,tnikkel,#layout

When we're replicating a fixed-positioned frame that happens to contain a
subdocument, we were losing track of the primary frame of the element.

The regressing bug made us rely a bit more in the frame tree structure
here (of FindContainerFrame() and such), but this was a pre-existing bug.

This prevents us from clobbering it and is enough to fix the crash and restore
pre-regression behavior, so it is the minimal thing to uplift.

Further clean-ups incoming.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2010601 - Deal with not being the primary subdoc frame in some cases. r=#layout

It's very sketchy to propagate state from non-primary frames to the
frameloader.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2010601 - Add a crashtest. r=#layout

Seems better to test with both a PDF and another random object.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5308a95cb395
https://hg.mozilla.org/integration/autoland/rev/4e363a6e7f05
Avoid losing track of primary frame in nsSubDocumentFrame::Init. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/074c24bf134f
https://hg.mozilla.org/integration/autoland/rev/b0736561f926
Deal with not being the primary subdoc frame in some cases. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/9bae6dec5807
https://hg.mozilla.org/integration/autoland/rev/533ca936f137
Add a crashtest. r=layout-reviewers,dshin

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/57206 for changes under testing/web-platform/tests

Comment 24

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4e363a6e7f05
https://hg.mozilla.org/mozilla-central/rev/b0736561f926
https://hg.mozilla.org/mozilla-central/rev/533ca936f137

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 26

[Donal Meehan [:dmeehan]]

:emilio, can you add beta and release uplift requests when you have a moment?

Comment 27

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9538142 [details]
Bug 2010601 - Avoid losing track of primary frame in nsSubDocumentFrame::Init. r=dholbert,jfkthame,tnikkel,#layout

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: crashes
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 7 and comment 11
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial-ish patch
• String changes made/needed: none
• Is Android affected?: Yes

Comment 28

[Emilio Cobos Álvarez [:emilio]]

The second patch we don't really need to uplift I think.

Comment 29

[Donal Meehan [:dmeehan]]

Comment on attachment 9538142 [details]
Bug 2010601 - Avoid losing track of primary frame in nsSubDocumentFrame::Init. r=dholbert,jfkthame,tnikkel,#layout

Approved for 148.0b4

Comment 30

[Donal Meehan [:dmeehan]]

Comment on attachment 9538144 [details]
Bug 2010601 - Add a crashtest. r=#layout

Approved for 148.0b4

Comment 31

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/68393ec8d807
https://hg.mozilla.org/releases/mozilla-beta/rev/998562bcd5d5
https://github.com/mozilla-firefox/firefox/commit/ae3b1d02644d
https://hg.mozilla.org/releases/mozilla-beta/rev/c95e7f50d46d

Comment 32

[Monica Chiorean]

I was able to reproduce the crash(https://crash-stats.mozilla.org/report/index/1efb86e0-e1fb-4607-9d6a-0a7d10260119) on Win11x64 using Firefox build 147.0 and steps from comment#7 and testcase from description.
Verified as fixed on Win11x64 using FF builds:149.0a1(20260118214518). Waiting for 148.0b4.

Comment 33

[Monica Chiorean]

Verified as fixed on Win11x64 using FF build 148.0b4.

Comment 34

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/19143efc2fb7
https://hg.mozilla.org/releases/mozilla-beta/rev/d096a90ea6b5

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9538142 [details]
Bug 2010601 - Avoid losing track of primary frame in nsSubDocumentFrame::Init. r=dholbert,jfkthame,tnikkel,#layout

Approved for 147.0.2

Comment 36

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e2620af3280f
https://hg.mozilla.org/releases/mozilla-release/rev/b1939723d731
https://github.com/mozilla-firefox/firefox/commit/12fcc98652b7
https://hg.mozilla.org/releases/mozilla-release/rev/fc816c3343b2

Comment 37

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e93907da6530
https://hg.mozilla.org/releases/mozilla-release/rev/df4c7ecb7497

Comment 38

[Monica Chiorean]

Verified as fixed on Win11x64 using FF build 147.0.2.