1140268 - "ASSERTION: no containing block" and crash [@ nsHTMLReflowState::InitConstraints] with MathML

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: no containing block: 'nullptr != cbrs', file layout/generic/nsHTMLReflowState.cpp, line 1980

Crash [@ nsHTMLReflowState::InitConstraints] trying to call nsCSSOffsetState::GetWritingMode on a null pointer.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Bob Clary [:bc] (inactive)]

Bughunter can reproduce this crash on 32bit and 64bit Windows and Linux and OSX on all three branches Beta/44, Aurora/45 and Nightly/45.

Comment 3

[Marc Verbiest [:mbv]]

Attachment: bug1140268.patch

I'm not sure about this patch, for example is it necessary to reflow if the offsetheight is accessed?

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8731689 [details] [diff] [review]
bug1140268.patch

Thanks for the patch, Marc.

I don't really know this code, but I suspect parentReflowState should only be
used for the containing block if its frame is the containing block.

Why does the parentReflowState not have a mCBReflowState?

Guessing jfkthame might be suitable reviewer based on
https://hg.mozilla.org/mozilla-central/rev/a5bc284ebe1c#l2.82

Comment 5

[Jonathan Kew [:jfkthame]]

This is a "dummy" state constructed by RestyleManager::RecomputePosition here:

https://dxr.mozilla.org/mozilla-central/rev/4037eb98974db1b1e0b5012c8a7f3a36428eaa11/layout/base/RestyleManager.cpp#484-493

That state has no mCBReflowState, as it was constructed using the root reflowState constructor. It is then passed in as the parent reflowState here:

https://dxr.mozilla.org/mozilla-central/rev/4037eb98974db1b1e0b5012c8a7f3a36428eaa11/layout/base/RestyleManager.cpp#518-519

And it's within that constructor for the child that we run into this assertion, and then crash.

My guess is that we've probably had the potential to hit the assertion (in principle, given the right testcase and a debug build) for a long time, but we used to survive because we didn't actually depend on accessing the cbrs. It's only become a potential crash with the conversion to logical coordinates, which means we try to access the cbrs's writing-mode.

Given the comment where the "dummy" reflow state is created in RecomputePosition, I wonder whether we should be using the containing block rather than the parent there? If we do that, then the test

  parentReflowState->frame == frame->GetContainingBlock()

in InitCBReflowState() will be true when we're setting up the child state, and we don't end up in this unfortunate situation of trying to use an mCBReflowState that doesn't exist.

I tried pushing a test run with such a patch (after confirming that it fixes this testcase), and so far it seems to be passing tests OK... https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d3a1a896f9.

But I don't know much about this RestyleManager stuff, and whether the above actually makes reasonable sense.... so I'd like to see what :dbaron has to say.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8731689 [details] [diff] [review]
bug1140268.patch

OK, I've been looking through this code some more, and while this does prevent the assertion/crash, I don't think it is strictly correct, as Karl suggested. (It's a great start, though, pointing a finger towards the source of the problem -- thanks!)

The parentReflowState here is a "dummy" state that was created in RecomputePosition as a root reflow state, and as a result, it didn't get its proper mCBReflowState set up. And that's why it can't pass that on to its child.

So I think the better solution here is to ensure that when this "dummy" parent state is created, its mCBReflowState *does* get set appropriately.

Dropping ni? on dbaron for now; I'll post a suggested patch for review instead.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Crashtest

Crashtest based on the testcase here. Asserts (and then crashes) with current trunk.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Set up mCBReflowState for the 'bogus' parent reflow state used for RestyleManager::RecomputePosition

And this fixes the issue by setting up the parent's mCBReflowState, as discussed above.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8733857 [details] [diff] [review]
Set up mCBReflowState for the 'bogus' parent reflow state used for RestyleManager::RecomputePosition

>+  // The bogus parent state here was created with no parent state of its own,
>+  // and therefore it won't have an mCBReflowState set up.
>+  // But we may need one (for InitCBReflowState in a child state), so let's
>+  // try to create one here.
>+  Maybe<nsHTMLReflowState> cbReflowState;
>+  nsIFrame* cbFrame = parentFrame->GetContainingBlock();
>+  if (cbFrame) {

Maybe bother with this only if:

  cbFrame && (aFrame->GetContainingBlock() != parentFrame ||
              parentFrame->GetType() == nsGkAtoms::tableFrame)

since those are the conditions for which the reflowState would end up with a null mCBReflowState.


r=dbaron, preferably with that change, I think

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b8c2bc37283beaaf9a53a80617c2eb431e2d56
Bug 1140268 - Crashtest. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffff3be72a1dd4117ac1880acad402caf517b5f2
Bug 1140268 - Set up mCBReflowState for the 'bogus' parent reflow state used for RestyleManager::RecomputePosition. r=dbaron

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37b8c2bc3728
https://hg.mozilla.org/mozilla-central/rev/ffff3be72a1d