Closed Bug 1535986 Opened 5 years ago Closed 5 years ago

Crash [@ mozilla::ReflowInput::InitAbsoluteConstraints ]

Categories

(Core :: Layout: Grid, defect)

Product:
Core
Component:
Layout: Grid
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 + fixed
firefox68 --- fixed

People

(Reporter: tanner, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, regression, testcase-wanted)

Crash Data

Attachments

(1 file)

Bug 1535986 - Skip recomputing the position for frames that have a pending reflow. r=dholbert
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60-
RyanVM
: approval-mozilla-geckoview66-
Details | Review

I can't put a testcase together at the moment, but you can (hopefully) reproduce by going to https://www.roberthalf.com/jobs/all-jobs/52405 and choosing any option from the "Narrow your results" menu.

This seems to be related to bug 1520584.

16:31.54 INFO: Last good revision: f34ff529f92e55627ddbc6f1304f87eaaf5bb341
16:31.54 INFO: First bad revision: e12caed89db3e037deb713d8a22a5c9cfa542b1e
16:31.55 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f34ff529f92e55627ddbc6f1304f87eaaf5bb341&tochange=e12caed89db3e037deb713d8a22a5c9cfa542b1e

bp-ed103d17-7897-4ff0-940a-83c6b0190318
bp-20a35314-bbf9-4e79-b594-5f6b70190318
bp-6296cb4f-17dd-4793-8404-6b6c60190318
bp-d1bb391d-e98e-4abe-89e8-fc5660190318

[Tracking Requested - why for this release]:

Can repro, thanks for the regression range!

Mats, do you have cycles to look at this? Otherwise I can try to take a look sometime this week.

[Tracking Requested - why for this release]: Reproducible crash introduced in this release.

Blocks: 1520584
status-firefox66: --- → unaffected
status-firefox67: --- → affected
tracking-firefox67: --- → ?
Flags: needinfo?(mats)

Tracking for 67, if you can repro this and fix it, I'll take the uplift in beta 67.

tracking-firefox67: ? → +
Crash Signature: [@ mozilla::ReflowInput::InitAbsoluteConstraints]
Keywords: crash

I get an assertion in a DEBUG build:
Assertion failure: cb (this method must only be called on grid items, and the grid container should've reflowed this item by now and set up cb), at layout/generic/nsGridContainerFrame.cpp:2510
I'll take a look...

Flags: needinfo?(mats)

So, at the time of that assertion we have this stack:

#1  mozilla::ReflowInput::InitAbsoluteConstraints
#2  mozilla::ReflowInput::InitConstraints
#3  mozilla::ReflowInput::Init
#4  mozilla::ReflowInput::ReflowInput
#5  mozilla::RecomputePosition
#6  mozilla::RestyleManager::ProcessRestyledFrames
...

It seems this ReflowInput is created (in RecomputePosition) to determine
if we can optimize away a reflow and instead simply move the position
of an abs.pos. frame in response to some style change.

The frame tree looks like this:

(gdb) call aCBReflowInput->mFrame->DumpFrameTreeLimited()
GridContainer(fieldset)(3)@5555581210b8 parent=555558121010 {0,0,0,0} [state=0000002000041602] [content=555556dc9a60] [cs=555559312ff8:-moz-fieldset-content]<
  Placeholder(span)(1)@555557c24720 parent=5555581210b8 next=555557c24938 {0,0,0,0} [state=0000000000200402] [content=55555e3777f0] [cs=555557654488:-moz-oof-placeholder] outOfFlowFrame=HTMLScroll(span)(1)@555558121168
  Placeholder(label)(3)@555557c24938 parent=5555581210b8 {0,0,0,0} [state=0000000000200402] [content=55555c0615e0] [cs=555557654488:-moz-oof-placeholder] outOfFlowFrame=Block(label)(3)@555557f53020
>
AbsoluteList 5555596b4d10 <
  HTMLScroll(span)(1)@555558121168 parent=5555581210b8 next=555557f53020 {0,0,0,0} [state=0080060000000502] [content=55555e3777f0] [cs=7fff7400e758]<
    Block(span)(1)@555557f51e40 parent=555558121168 {0,0,0,0} [state=0000002000d00602] [content=55555e3777f0] [cs=555558846f28:-moz-scrolled-content]<
      line 55555842a730: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} <
        Text(0)"Location"@555557727ca8 parent=555557f51e40 {0,0,0,0} [state=0000000000000402] [content=55555e39cc20] [cs=555558d9bbf8:-moz-text] [run=0][0,8,T] 
      >
    >
  >
  Block(label)(3)@555557f53020 parent=5555581210b8 {0,0,0,0} [state=0040062000d00702] [content=55555c0615e0] [cs=55555e4f5e98]<
    line 555558418380: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} <
      Text(0)"Location"@555557f51cb0 parent=555557f53020 {0,0,0,0} [state=0000000000000402] [content=55555e3aad70] [cs=55555e4f52a8:-moz-text] [run=0][0,8,T] 
    >
  >
>

with aFrame=0x555557f53020

The interesting bit is that 'state' ends with '2' so NS_FRAME_FIRST_REFLOW
is set which means none of these frames have ever been reflowed.

Hmm, it seems to me that if NS_FRAME_FIRST_REFLOW is set we should
return 'true' upfront (before doing expensive stuff like setting up
a ReflowInput). Given that we will reflow this frame at some
point it doesn't really matter what position we move it to.
Also, at the time we do try to move it we should also check
NS_FRAME_FIRST_REFLOW and skip that code - it's pointless to move
frames around that have never been reflowed.

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d446a6e08c
Skip recomputing the position for frames that have a pending reflow.  r=dholbert

Thanks for looking into this Mats :)

Err, though, any chance to land a test for this? If you're busy I can try to reduce the page tomorrow or something.

Flags: needinfo?(mats)

The page looked rather complicated so I didn't have time...
But yeah, ideally we should land a test for this eventually.

(I'd prefer if you review the new list-item / ::marker patches though. ;) )

Flags: needinfo?(mats)
Keywords: testcase-wanted

That's fair, I figured :P

Let's try something else before somebody has to go and manually reduce that website... Jason, have you or anyone on your team seen a crash like this lately?

On a debug build, it asserts with the message mentioned on comment 3:

Assertion failure: cb (this method must only be called on grid items, and the grid container should've reflowed this item by now and set up cb), at layout/generic/nsGridContainerFrame.cpp:2510

And on opt it should crash with a null deref in InitAbsoluteConstraints.

Thanks!

Flags: needinfo?(jkratzer)

No, unfortunately nothing with that exact assertion. However, I do see a recent log for the following assertion on debug builds with a similar stack:

Assertion failure: aContainingBlockBSize != nscoord((1 << 30) - 1) || !aCoord.HasPercent() (caller must deal with %% of unconstrained block-size), at /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.h:1502

0|0|libxul.so|nsLayoutUtils::ComputeBSizeValue(int, int, mozilla::StyleLengthPercentage const&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsLayoutUtils.h:aecc707103590186a2a7fa5d002849f443417e59|1501|0x0
0|1|libxul.so|mozilla::ReflowInput::CalculateHypotheticalPosition(nsPresContext*, nsPlaceholderFrame*, mozilla::ReflowInput const*, nsHypotheticalPosition&, mozilla::LayoutFrameType) const|hg:hg.mozilla.org/mozilla-central:layout/generic/ReflowInput.cpp:aecc707103590186a2a7fa5d002849f443417e59|1610|0xd
0|2|libxul.so|mozilla::ReflowInput::InitAbsoluteConstraints(nsPresContext*, mozilla::ReflowInput const*, mozilla::LogicalSize const&, mozilla::LayoutFrameType)|hg:hg.mozilla.org/mozilla-central:layout/generic/ReflowInput.cpp:aecc707103590186a2a7fa5d002849f443417e59|1680|0x20
0|3|libxul.so|mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::LogicalSize const&, nsMargin const*, nsMargin const*, mozilla::LayoutFrameType)|hg:hg.mozilla.org/mozilla-central:layout/generic/ReflowInput.cpp:aecc707103590186a2a7fa5d002849f443417e59|2433|0x5
0|4|libxul.so|mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*)|hg:hg.mozilla.org/mozilla-central:layout/generic/ReflowInput.cpp:aecc707103590186a2a7fa5d002849f443417e59|417|0x1a
0|5|libxul.so|nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, mozilla::ReflowInput const&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsIFrame*, nsReflowStatus&, nsOverflowAreas*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsAbsoluteContainingBlock.cpp:aecc707103590186a2a7fa5d002849f443417e59|667|0x5
0|6|libxul.so|nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, mozilla::ReflowInput const&, nsReflowStatus&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsOverflowAreas*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsAbsoluteContainingBlock.cpp:aecc707103590186a2a7fa5d002849f443417e59|159|0x35
0|7|libxul.so|nsFrame::ReflowAbsoluteFrames(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, bool)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|6344|0xc
0|8|libxul.so|nsInlineFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsInlineFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|364|0x1e
0|9|libxul.so|nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsLineLayout.cpp:aecc707103590186a2a7fa5d002849f443417e59|880|0x21
0|10|libxul.so|nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|4097|0x14
0|11|libxul.so|nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|3900|0x2d
0|12|libxul.so|nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|3787|0x41
0|13|libxul.so|nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|2806|0x20
0|14|libxul.so|nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|2349|0x20
0|15|libxul.so|nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsBlockFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|1207|0xf
0|16|libxul.so|nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsContainerFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|890|0x1d
0|17|libxul.so|nsTableCellFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)|hg:hg.mozilla.org/mozilla-central:layout/tables/nsTableCellFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|860|0x48
0|18|libxul.so|nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*)|hg:hg.mozilla.org/mozilla-central:layout/generic/nsContainerFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|890|0x1d
0|19|libxul.so|nsTableRowFrame::ReflowChildren(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsTableFrame&, nsReflowStatus&)|hg:hg.mozilla.org/mozilla-central:layout/tables/nsTableRowFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|803|0x3b
0|20|libxul.so|nsTableRowFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)|hg:hg.mozilla.org/mozilla-central:layout/tables/nsTableRowFrame.cpp:aecc707103590186a2a7fa5d002849f443417e59|1001|0x1b
...truncated...

Let me know if this will help and I'll go ahead and reduce it.

Flags: needinfo?(jkratzer)

It might be easier to reduce it using a non-DEBUG build then, and check for a crash.
(or use a DEBUG build but set the environment variable XPCOM_DEBUG_BREAK=warn first)

Running that testcase with a non-debug build fails to crash and a debug build with XPCOM_DEBUG_BREAK=warn returns the same assertion/stack.

https://hg.mozilla.org/mozilla-central/rev/53d446a6e08c

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → mats

@mats: Per comment 2, do you want to request uplift?

Flags: needinfo?(mats)

It's not a security issue so an uplift isn't strictly necessary.
That said, I think it's a reasonably low-risk fix.

Flags: needinfo?(mats)

Hi Mats, since 67 is marked as affected and your last comment deems it low-risk, could you please nominate a patch for uplift to Beta67?

Flags: needinfo?(mats)

Also, the same question for ESR60.7 uplift.

Comment on attachment 9051874 [details]
Bug 1535986 - Skip recomputing the position for frames that have a pending reflow. r=dholbert

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1520584
  • User impact if declined: null-pointer crash in rare circumstances
  • Is this code covered by automated tests?: Yes
  • 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):
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: null-pointer crash in rare circumstances
  • User impact if declined: null-pointer crash in rare circumstances
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:

GeckoView Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for consideration: null-pointer crash in rare circumstances
  • User impact if declined: null-pointer crash in rare circumstances
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
Flags: needinfo?(mats)
Attachment #9051874 - Flags: approval-mozilla-geckoview66?
Attachment #9051874 - Flags: approval-mozilla-esr60?
Attachment #9051874 - Flags: approval-mozilla-beta?

Comment on attachment 9051874 [details]
Bug 1535986 - Skip recomputing the position for frames that have a pending reflow. r=dholbert

Crash fix and a non-risky patch, uplift approved for 67 beta 6, thanks.

Attachment #9051874 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9051874 [details]
Bug 1535986 - Skip recomputing the position for frames that have a pending reflow. r=dholbert

AFAICT, this crash doesn't have significant volume warranting ESR60 or GV66 uplift.

Attachment #9051874 - Flags: approval-mozilla-geckoview66?
Attachment #9051874 - Flags: approval-mozilla-geckoview66-
Attachment #9051874 - Flags: approval-mozilla-esr60?
Attachment #9051874 - Flags: approval-mozilla-esr60-
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: