997709 - crash in mozalloc_abort(char const*) | Abort | NS_DebugBreak | nsCSSFrameConstructor::CreateContinuingFrame(nsPresContext*, nsIFrame*, nsIFrame*, bool) on print/print preview

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: crash1.htm

See testcase, which crashes on printing/print preview.

This bug was filed from the Socorro interface and is 
report bp-8d181892-3e96-4e09-9445-fb3092140417.
=============================================================
0 	libmozalloc.dylib 	mozalloc_abort(char const*) 	memory/mozalloc/mozalloc_abort.cpp
1 	XUL 	Abort 	xpcom/base/nsDebugImpl.cpp
2 	XUL 	NS_DebugBreak 	xpcom/base/nsDebugImpl.cpp
3 	XUL 	nsCSSFrameConstructor::CreateContinuingFrame(nsPresContext*, nsIFrame*, nsIFrame*, bool) 	layout/base/nsCSSFrameConstructor.cpp
4 	XUL 	nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp
5 	XUL 	nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) 	layout/generic/nsBlockFrame.cpp
6 	XUL 	nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/generic/nsBlockFrame.cpp
7 	XUL 	nsComboboxControlFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/forms/nsComboboxControlFrame.cpp
8 	XUL 	nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, nsHTMLReflowState const&, nsRect const&, bool, nsIFrame*, unsigned int&, nsOverflowAreas*) 	layout/generic/nsAbsoluteContainingBlock.cpp
9 	XUL 	nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, nsHTMLReflowState const&, unsigned int&, nsRect const&, bool, bool, bool, nsOverflowAreas*) 	layout/generic/nsAbsoluteContainingBlock.cpp
10 	XUL 	nsFrame::ReflowAbsoluteFrames(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, bool) 	layout/generic/nsFrame.cpp

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 2

[Mats Palmgren (inactive)]

Attachment: Framedump + stack of Testcase #2 in Print Preview

We're trying to split a nsComboboxDisplayFrame which isn't supported.

The frame dump shows the root of the problem: note that the frame
is on its own line (with state=block) and the dropdown button
on the next line.  A single line block isn't splittable but
a two-line block is.  The reason is that the (red) frame has been
blockified in ApplyStyleFixups because the parent context has
display:flex.  This not only causes the Print crash but also wrong
layout as can be seen by just loading Testcase #2 (but don't
Print/Preview it!).  This is a general problem with that code
when the frame ctor ignores display:flex/grid when constructing
the frame.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - nsComboboxDisplayFrame can't be split so its reflow status is always NS_FRAME_COMPLETE

This fixes the crash, but the layout is still wrong.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - Prevent the nsComboboxDisplayFrame from being blockified if the parent context has display:flex/grid

This fixes the layout by suppressing the ApplyStyleFixups blockification
for the anonymous nsComboboxDisplayFrame.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42e215a00ee

Comment 6

[Mats Palmgren (inactive)]

It wouldn't surprise me if other frame types have wrong layout with display:flex/grid
due to this bogus blockification.

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Mats Palmgren (:mats) from comment #6)
> It wouldn't surprise me if other frame types have wrong layout with
> display:flex/grid due to this bogus blockification.

Yeah.  Is there any general rule that we could apply here to determine whether the blockification should be skipped?  Nothing comes to me, but if there is, it might make sense to add the logic for skipping blockification to ApplyStyleFixups itself.

Will you audit other frames to make sure we're not blockifying other anonymous box children unnecessarily?

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8610265 [details] [diff] [review]
part 2 - Prevent the nsComboboxDisplayFrame from being blockified if the parent context has display:flex/grid

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

::: layout/style/nsStyleSet.h
@@ +193,5 @@
>                            mozilla::dom::Element* aPseudoElement = nullptr);
>  
> +  /**
> +   * Bit-flags that can be passed to ResolveAnonymousBoxStyle, GetContext,
> +   * in its parameter 'aFlags'.

s/, GetContext, in its/ and GetContext in their/

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31e905c2b1a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d83527b03fea
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e726479c08

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31e905c2b1a6
https://hg.mozilla.org/mozilla-central/rev/d83527b03fea
https://hg.mozilla.org/mozilla-central/rev/a7e726479c08

Comment 11

[Mats Palmgren (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> (In reply to Mats Palmgren (:mats) from comment #6)
> > It wouldn't surprise me if other frame types have wrong layout with
> > display:flex/grid due to this bogus blockification.
> 
> Yeah.  Is there any general rule that we could apply here to determine
> whether the blockification should be skipped?  Nothing comes to me, but if
> there is, it might make sense to add the logic for skipping blockification
> to ApplyStyleFixups itself.

I don't see simple solution for this specific case since we would need
the parent frame type to determine if its 'display' was "ignored" or not.

> Will you audit other frames to make sure we're not blockifying other
> anonymous box children unnecessarily?

Filed bug 1169627.

Comment 12

[Martijn Wargers (dead)]

Verified fixed, using the latest trunk build on MacOSX10.9.5