Closed Bug 1140268 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: MathML, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jruderman, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, crash, testcase)

Attachments

(5 files)

Attached file 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.
Attached file stack
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.
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch bug1140268.patchSplinter Review
I'm not sure about this patch, for example is it necessary to reflow if the offsetheight is accessed?
Attachment #8731689 - Flags: review?(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
Attachment #8731689 - Flags: review?(karlt) → review?(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.
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
Attachment #8731689 - Flags: review?(jfkthame)
Attached patch CrashtestSplinter Review
Crashtest based on the testcase here. Asserts (and then crashes) with current trunk.
Attachment #8733856 - Flags: review?(dbaron)
And this fixes the issue by setting up the parent's mCBReflowState, as discussed above.
Attachment #8733857 - Flags: review?(dbaron)
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
Attachment #8733857 - Flags: review?(dbaron) → review+
Attachment #8733856 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/37b8c2bc3728
https://hg.mozilla.org/mozilla-central/rev/ffff3be72a1d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.