Closed Bug 1304441 Opened 8 years ago Closed 8 years ago

Assertion failure: !summary || !summary->IsMainSummary()

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: truber, Assigned: TYLin)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(6 files, 1 obsolete file)

Attached file testcase.html
Attached testcase asserts in mozilla-central revision 62f79d676e0e

Assertion failure: !summary || !summary->IsMainSummary() (Rest of the children are neither summary elements northe main summary!), at /home/worker/workspace/build/src/layout/generic/DetailsFrame.cpp:82
Attached file stacktrace
Flags: in-testsuite?
TY, could you have a look at this?
Flags: needinfo?(tlin)
Sure.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
Component: CSS Parsing and Computation → Layout
Boris, I'll need your help to review my changes when you're less busy.  

Part 1 & 2 are just clean up. The asserts of this bug happens in the integrity check of main summary frame in Details::SetInitialChildList(), which is fixed by part 3.  And part 4 fixes some portion of a real bug involving ::first-line.
Flags: needinfo?(bzbarsky)
Comment on attachment 8794763 [details]
Bug 1304441 Part 1 - Remove unneeded check when constructing frame for summary.

https://reviewboard.mozilla.org/r/81064/#review80050

::: layout/base/nsCSSFrameConstructor.cpp:3564
(Diff revision 1)
>      // positioned legends we want to construct by display type and
>      // not do special legend stuff.
>      return nullptr;
>    }
>  
> -  if (aTag == nsGkAtoms::details || aTag == nsGkAtoms::summary) {
> +  if (aTag == nsGkAtoms::details) {

Should probably make this use && instead of two nested ifs.

r=me
Attachment #8794763 - Flags: review+
Comment on attachment 8794764 [details]
Bug 1304441 Part 2 - Extract main summary checking code to a function.

https://reviewboard.mozilla.org/r/81066/#review80056

::: layout/generic/DetailsFrame.cpp:94
(Diff revision 1)
> -          break;
> +        break;
> -        }
> +      }
> -      } else {
> +    } else {
> -        MOZ_ASSERT(!summary || !summary->IsMainSummary(),
> +      MOZ_ASSERT(!summary || !summary->IsMainSummary(),
> -                   "Rest of the children are neither summary elements nor"
> +                 "Rest of the children are neither summary elements nor"
> -                   "the main summary!");
> +                 " the main summary!");

I'd moderately prefer the space at the end of the first string, not the beginning of the second one.  But either way is fine.

r=me
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review80058

In the changeset header:
> When specifing

"When specifying"

::: layout/generic/DetailsFrame.cpp:90
(Diff revision 1)
>        HTMLSummaryElement::FromContent(child->GetContent());
>  
>      if (child == aFrameList.FirstChild()) {
>        if (summary && summary->IsMainSummary()) {
> -        break;
> +        return true;
> +      } else if (child->GetType() == nsGkAtoms::lineFrame) {

So why is first-line special?  For example, why wouldn't we want to descend into arbitrary anonymous frames here?

::: layout/generic/DetailsFrame.cpp:99
(Diff revision 1)
> +          return true;
> +        }
>        }
>      } else {
>        MOZ_ASSERT(!summary || !summary->IsMainSummary(),
>                   "Rest of the children are neither summary elements nor"

Now that I think about this assert more, it should probably say:

  "Rest of the children are either not summary element or are not the main summary"
  
because that's what it's testing...

I'd really like to understand why we're specializing the check for first-line here...
Comment on attachment 8794766 [details]
Bug 1304441 Part 4 - Fix rendering error if summary has ib-split.

https://reviewboard.mozilla.org/r/81070/#review80062

::: layout/generic/DetailsFrame.cpp:67
(Diff revision 1)
> -          // the list.
> -          aChildList.RemoveFrame(child);
> -          aChildList.InsertFrame(nullptr, nullptr, child);
> +          // first continuation or ib-split sibling, some of the previous
> +          // sibling might be wrapped in a wrapper frame like
> +          // nsFirstLetterFrame. Because we do not deal perfectly in this case,
> +          // we move all the main summary frames only if they're all direct
> +          // children of the details frame.
> +          if (nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(child)) {

So... the fact that we _don't_ do this right in the first-line case is a bit weird, no?  In particular, I think the frame tree you end up with is pretty broken.  That really worries me from a security perspective.

Can you remind me why we're doing this here instead of doing it on frame construction items?  If we did it on the latter, we would automatically get the right behavior for the first-line case, no?  Also for cases when the <summary> is style="display: table-cell" while the <details> is "display:block". Right now we seem to handle those wrong.
Attachment #8794766 - Flags: review-
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review80058

> So why is first-line special?  For example, why wouldn't we want to descend into arbitrary anonymous frames here?

We probabily should, but I only check ::first-line here simply because I cannot think of a case that summary frame could be wrapper by other type of frames or an anonymous frame.  And nsFirstLineFrame is not an anonymous block frame.  For example, the details frame does not handle ib-split even if it has "display: inline" so no summary frame won't be wrapped in an anonymous block.  The similar pseudo ::first-letter [1] does not wrap summary either.  Do you have other examples?

> Now that I think about this assert more, it should probably say:
> 
>   "Rest of the children are either not summary element or are not the main summary"
>   
> because that's what it's testing...

Uh, I must misunderstood the grammar.  The original assert means `!summary && !summary->IsMainSummary()`, right?  So let's change it in part 2 since it touches the string.
Comment on attachment 8794766 [details]
Bug 1304441 Part 4 - Fix rendering error if summary has ib-split.

https://reviewboard.mozilla.org/r/81070/#review80062

> So... the fact that we _don't_ do this right in the first-line case is a bit weird, no?  In particular, I think the frame tree you end up with is pretty broken.  That really worries me from a security perspective.
> 
> Can you remind me why we're doing this here instead of doing it on frame construction items?  If we did it on the latter, we would automatically get the right behavior for the first-line case, no?  Also for cases when the <summary> is style="display: table-cell" while the <details> is "display:block". Right now we seem to handle those wrong.

Yes, the frame tree is still very broken with this patch.

I think we've never think of moving the summary on frame construction items in the past.  To be precise, do you mean that we could move the frame construction item of the main summary somewhere before actually generating frames before [1] in nsCSSFrameConstructor::ProcessChildren()?  If it works, then the subsequent WrapFramesInFirstLineFrame() should work automatically.

[1] http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/layout/base/nsCSSFrameConstructor.cpp#10758
> simply because I cannot think of a case that summary frame could be wrapper by other type of frames or an anonymous frame.

  <details><summary style="display: table-cell"></summary></details>

should do it.  See also comment 12.

An interesting question is whether such styling should disable the "move the summary to the beginning" behavior (like floating/positioning does for legends in fieldsets)...  That seems like a spec issue.  Worth checking what other implementations do.  In particular, if I style my summary as a table-cell, it would be pretty weird to move it to some other random place in the <details>, away from its neighboring table cells.

I really wonder how much the spec has been thought through here.

> The original assert means `!summary && !summary->IsMainSummary()`, right? 

Yes.

> Yes, the frame tree is still very broken with this patch.

Is it broken enough that this could be a security problem?  I'm really worried about violating invariants like this.... Put another way, which branches do we need to backport the fix to?  ;)

> I think we've never think of moving the summary on frame construction items in the past.

Huh.  I wonder why not.  I should have thought of it.  OK.

At least in part this depends on what behavior we want in cases when the summary has interesting display styles and whatnot, of course.

> To be precise, do you mean that we could move the frame construction item
> of the main summary somewhere before actually generating frames before [1]
> in nsCSSFrameConstructor::ProcessChildren()?

Yes, something like that.  It doesn't make me very happy to add a special case like that for <details>, but it might be simplest, depending on which behavior we really want here.

I just checked on how we handle this for the fieldset/legend case, and it looks like the differences are:

1)  We don't support ::first-line on fieldset (precisely because of this issue; see bug 431520).
2)  We don't really support display styling on the primary legend of a fieldset.
    So if you style it display:table-cell that effectively gets ignored.

so this situation never really arises...
Flags: needinfo?(bzbarsky)
> An interesting question is whether such styling should disable the "move the
> summary to the beginning" behavior (like floating/positioning does for
> legends in fieldsets)...  That seems like a spec issue.  Worth checking what
> other implementations do.  In particular, if I style my summary as a
> table-cell, it would be pretty weird to move it to some other random place
> in the <details>, away from its neighboring table cells.

I make a test case "summary-display-styles.html" which has various display styles on summary elements. On Google Chrome, summary elements always behaves like a block. The spec says the first summary element should be in the first anonymous block [1] so the summary behaves like block outside anyway. 

Safari seems respect summary's display style in some ways, but its rendering is not very stable. For example, after closing and opening the details, and clicking on the "Details"string, summary sometimes renders as the second child.

Fieldset requires the legend satisfying certain conditions to be a "rendered legend" [2]. I wonder whether we need something like "rendered summary". I open a spec issue here. https://github.com/whatwg/html/issues/1839

> Is it broken enough that this could be a security problem?  I'm really
> worried about violating invariants like this.... Put another way, which
> branches do we need to backport the fix to?  ;)

Details and summary has just been released in 49. So if it does have a serious security problem, we'll need to backport the fix all the way to the release channel. But for now, we just render the summary at the wrong place in some case.

[1] https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
[2] https://html.spec.whatwg.org/multipage/rendering.html#the-fieldset-and-legend-elements"
Regardless of the spec issue, I guess what I can do now is hacking nsCSSFrameConstructor::ProcessChildren() to see if moving summary on the frame construction list makes the ib-split summary and ::first-line correct.
Attachment #8794766 - Attachment is obsolete: true
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review81668

The idea to move summary to the front on frame construction item work quite well (See my patchset 2 part 3).  It still does what the spec currently says to *always* move the first summary to the front, but it should fix the broken frame tree.

::: layout/base/nsCSSFrameConstructor.h:845
(Diff revision 2)
>  
>      // aSuppressWhiteSpaceOptimizations is true if optimizations that
>      // skip constructing whitespace frames for this item or items
>      // around it cannot be performed.
>      // Also, the return value is always non-null, thanks to infallible 'new'.
>      FrameConstructionItem* AppendItem(const FrameConstructionData* aFCData,

Here is AppendItem()

::: layout/base/nsCSSFrameConstructor.h:880
(Diff revision 2)
> +      FrameConstructionItem* item =
> +        new FrameConstructionItem(aFCData, aContent, aTag, aNameSpaceID,
> +                                  aPendingBinding, aStyleContext,
> +                                  aSuppressWhiteSpaceOptimizations,
> +                                  aAnonChildren);
> +      PR_INSERT_LINK(item, &mItems);

This makes me want to rewrite FrameConstructionList to use mfbt/LinkedList.h.
While waiting for the spec [1] to discuss the desired behavior of summary elements under various display styles, we might still want to fix the "always move the first summary element to front" behavior so that the frame tree won't be broken.

Boris, would you help review my patchset 2?  

[1] https://github.com/whatwg/html/issues/1839
Flags: needinfo?(bzbarsky)
> This makes me want to rewrite FrameConstructionList to use mfbt/LinkedList.h.

Doesn't seem unreasonable; this code predates LinkedList existing.  ;)
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review81756

::: layout/base/nsCSSFrameConstructor.cpp:5904
(Diff revision 2)
> -  FrameConstructionItem* item =
> -    aItems.AppendItem(data, aContent, aTag, aNameSpaceID,
> +  FrameConstructionItem* item = nullptr;
> +  if (details && details->IsDetailsEnabled() && details->Open()) {
> +    auto* summary = HTMLSummaryElement::FromContentOrNull(aContent);
> +    if (summary && summary->IsMainSummary()) {
> +      // If details is open, the main summary needs to be rendered as if it is
> +      // the first child, so add the item to the front of the item list.

This still has a problem, though that's preexisting: this puts the summary before the ::before pseudo of the details element.

That's not what the spec says to do, and not what Blink and WebKit do.  They also don't hide ::before for a non-open details.

For uplift purposes, this is probably OK, but we should get a followup filed to address this, because I expect dynamic ::before changes on `<details>` are somewhat broken.  And perhaps another followup to rip all this out and replace with a shadow DOM thing once that's working; this last should depend on our shadow DOM bug.

::: layout/generic/DetailsFrame.cpp:71
(Diff revision 2)
>      if (child == aFrameList.FirstChild()) {
>        if (summary && summary->IsMainSummary()) {
> -        break;
> +        return true;
> +      } else if (child->GetContent() == GetContent()) {
> +        // The child frame's content is the same as our content, which means
> +        // it's a kind of wrapper frames. Descend into its child list to find

s/wrapper frames/wrapper frame/

r=me with those changes.
Flags: needinfo?(bzbarsky)
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review81800
Attachment #8794765 - Flags: review+
Comment on attachment 8794765 [details]
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list.

https://reviewboard.mozilla.org/r/81068/#review81756

> This still has a problem, though that's preexisting: this puts the summary before the ::before pseudo of the details element.
> 
> That's not what the spec says to do, and not what Blink and WebKit do.  They also don't hide ::before for a non-open details.
> 
> For uplift purposes, this is probably OK, but we should get a followup filed to address this, because I expect dynamic ::before changes on `<details>` are somewhat broken.  And perhaps another followup to rip all this out and replace with a shadow DOM thing once that's working; this last should depend on our shadow DOM bug.

I filed bug 1307679 to deal with ::before and ::after.

> And perhaps another followup to rip all this out and replace with a shadow DOM thing once that's working; this last should depend on our shadow DOM bug.

You remind me that we have bug 1258948 to prevent details and summary to be a shadow root. I don't fullly understand this, but I guess we could re-implement move-to-the-front summary by manipulating something like details element's shadow tree so that we don't have to deal with ::before on the frame construction item level. Is bug 1205323 tracking our shadow DOM work.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41d60821e3c
Part 1 - Remove unneeded check when constructing frame for summary. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5724ff0f7c
Part 2 - Extract main summary checking code to a function. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88ef7f3c2d5
Part 3 - Insert main summary's frame construction item at front of the list. r=bz
https://hg.mozilla.org/mozilla-central/rev/f41d60821e3c
https://hg.mozilla.org/mozilla-central/rev/9f5724ff0f7c
https://hg.mozilla.org/mozilla-central/rev/f88ef7f3c2d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
> but I guess we could re-implement move-to-the-front summary by manipulating something like details element's shadow tree

Right, that's basically what the spec says to do.

> Is bug 1205323 tracking our shadow DOM work.

Looks plausible.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: