Closed Bug 1278455 Opened 8 years ago Closed 8 years ago

[css-grid][css-flexbox] "ASSERTION: Reference frame mismatch" with display:grid/flex

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jruderman, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
With:
  user_pref("layout.event-regions.enabled", true);

###!!! ASSERTION: Reference frame mismatch: 'aBuilder->FindReferenceFrameFor(aFrame) == aBuilder->FindReferenceFrameFor(mFrame)', file layout/base/nsDisplayList.cpp, line 3578
Attached file stack
Also reproduces in e10s mode (which turns on that pref by default).
There's actually a rendering difference between e10s and non-e10s with this testcase. The Z appears further down.
Attached file frametree
The absolute frame is a child of the canvas frame, which is outside of the transformed subtree (rooted at the GridContainer frame).

Mats, do you know what is going on here?
Flags: needinfo?(mats)
It also occurs for display:flex, so maybe it has something to do with this:
https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsGridContainerFrame.cpp#l2212
https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsFlexContainerFrame.cpp#l2134
Does adding "|| !aFrame->GetParent()" to the condition there fix it?
Flags: needinfo?(mats)
Summary: "ASSERTION: Reference frame mismatch" with display:grid → "ASSERTION: Reference frame mismatch" with display:grid/flex
I think the abs pos child should have a parent inside the transformed subtree (since transforms induce an abs pos containing block). So I think we are going wrong before we even get to painting.
OK.  I'm not aware of any other exceptions for grid/flex off the top of my head.
Attached patch pushabsblock (obsolete) — Splinter Review
I looked into this very shallow-ly. It seems we are missing a call to PushAbsoluteContainingBlock when the root element is grid or flex. ConstructBlock and ConstructTable do that. This gives me the expected frame tree.
Attachment #8761419 - Flags: feedback?(dholbert)
Comment on attachment 8761419 [details] [diff] [review]
pushabsblock

Review of attachment 8761419 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this probably makes sense.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2568,5 @@
> +
> +    nsFrameConstructorSaveState absoluteSaveState;
> +    newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
> +    if (display->IsAbsPosContainingBlock(newFrame)) {
> +      state.PushAbsoluteContainingBlock(contentFrame, newFrame, absoluteSaveState);

(Extreme nit: this line is too long; wrap after "newFrame," probably)

@@ +2581,5 @@
> +
> +    nsFrameConstructorSaveState absoluteSaveState;
> +    newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
> +    if (display->IsAbsPosContainingBlock(newFrame)) {
> +      state.PushAbsoluteContainingBlock(contentFrame, newFrame, absoluteSaveState);

(Same here)
Attachment #8761419 - Flags: feedback?(dholbert) → feedback+
Comment on attachment 8761419 [details] [diff] [review]
pushabsblock

Hmm, does this actually fix the bug?
It seems odd because the nsFrameConstructorSaveState actually goes out
of scope before we process the children for grid/flexbox, which happens
in the "if (processChildren)" block further down.
~nsFrameConstructorSaveState restores the old state.

Shouldn't this go inside the processChildren block instead, so that it's
active during ProcessChildren() ?
Flags: needinfo?(tnikkel)
(In reply to Mats Palmgren (:mats) from comment #11)
> Hmm, does this actually fix the bug?
> It seems odd because the nsFrameConstructorSaveState actually goes out
> of scope before we process the children for grid/flexbox, which happens
> in the "if (processChildren)" block further down.
> ~nsFrameConstructorSaveState restores the old state.
> 
> Shouldn't this go inside the processChildren block instead, so that it's
> active during ProcessChildren() ?

I think you're right. But I did verify that it does in fact fix the frametree. The reason is that we don't seem to process the absolutely positioned div in the ProcessChildren call in ConstructDocElementFrame. We process it later during a flush. This does a ContentAppended call to create the frame, and it calls nsCSSFrameConstructor::GetAbsoluteContainingBlock to find the appropriate abs pos frame for the nsFrameConstructorState it creates. The gridcontainer frame has MarkAsAbsoluteContainingBlock called on it by the PushAbsoluteContainingBlock call I added. Without that call it isn't marked as an absolute containing block so nsCSSFrameConstructor::GetAbsoluteContainingBlock skips over it.
Flags: needinfo?(tnikkel)
Attached patch pushabsblockSplinter Review
Attachment #8761419 - Attachment is obsolete: true
Attachment #8761889 - Flags: review?(dholbert)
Comment on attachment 8761889 [details] [diff] [review]
pushabsblock

Review of attachment 8761889 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just a few nits:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2495,5 @@
>  
>    // Make sure to start any background image loads for the root element now.
>    styleContext->StartBackgroundImageLoads();
>  
> +  nsFrameConstructorSaveState parentAbsoluteSaveState;

Maybe s/parent/root/?   (or "rootFrame"?)  I'm not sure what nesting-level "parent" is referring to here. I think it's meant to refer to the root frame?

@@ +2570,5 @@
> +
> +    newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
> +    if (display->IsAbsPosContainingBlock(newFrame)) {
> +      state.PushAbsoluteContainingBlock(
> +        contentFrame, newFrame, absoluteSaveState);

Stylistic nit: this arg-indentation style is a bit unconventional.  It seems like the least-bad thing when it's necessary to avoid super-long lines or excessive linewrapping, but we don't seem to be in any danger of those here.

So I think this should just be unwrapped to:
      state.PushAbsoluteContainingBlock(contentFrame, newFrame,
                                        absoluteSaveState);

@@ +2583,5 @@
> +
> +    newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
> +    if (display->IsAbsPosContainingBlock(newFrame)) {
> +      state.PushAbsoluteContainingBlock(
> +        contentFrame, newFrame, absoluteSaveState);

(Same here.)
Attachment #8761889 - Flags: review?(dholbert) → review+
Blocks: css-grid
Summary: "ASSERTION: Reference frame mismatch" with display:grid/flex → [css-grid][css-flexbox] "ASSERTION: Reference frame mismatch" with display:grid/flex
(In reply to Daniel Holbert [:dholbert] from comment #14)
> > +  nsFrameConstructorSaveState parentAbsoluteSaveState;
> 
> Maybe s/parent/root/?   (or "rootFrame"?)  I'm not sure what nesting-level
> "parent" is referring to here. I think it's meant to refer to the root frame?

Going by this comment

https://dxr.mozilla.org/mozilla-central/rev/ddb6b30149221f00eb5dd180530e9033093d4c2b/layout/base/nsCSSFrameConstructor.cpp#2759

the most accurate name would be docElementContainingBlockAbsoluteSaveState.

We have the root frame and the root element frame (which are different frames), but this save state is for neither, so "root" or "root element" would be misleading.
Assignee: nobody → tnikkel
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f0406a8595
If the root element is grid or flex, allow it to be an absolute containing block. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/31f0406a8595
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: