370933 - "ASSERTION: aBlock must be a block" with <input type="file">

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jesse Ruderman]

Attachment: tiny testcase

Many pages, including
  https://bugzilla.mozilla.org/attachment.cgi?bugid=12345&action=enter
and
  https://bugzilla.mozilla.org/query.cgi?format=advanced
trigger
  ###!!! ASSERTION: aBlock must be a block: 'blockFrame', file layout/generic/nsBlockFrame.cpp

I reduced the former to just <input type="file">.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase, <legend>

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase, <isindex>

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

nsFileControlFrame, nsIsIndexFrame and nsLegendFrame are subclasses of
nsAreaFrame, so they should use nsAreaFrame::QueryInterface() at the end of
their QI impl method, not nsHTMLContainerFrame::QueryInterface().

The fix is nsHTMLContainerFrame -> nsAreaFrame, the rest is just a bit
of cleanup.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255726 [details] [diff] [review]
Patch rev. 1

Makes sense, and good catch!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255726 [details] [diff] [review]
Patch rev. 1

Actually, I think might confuse code that's outside these controls and expects them to actually be blocks in some sane way if they QI to that interface.

Comment 6

[Boris Zbarsky [:bzbarsky]]

roc, do you think it's safe for random block-like replaced elements to QI to the block CID?  Or should we just remove this assert?

Though <legend> probably _should_ QI to a block, as far as that goes, no matter what.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we've been using QI to mean "is nsBlockFrame or a subclass" and I think that's appropriate.

We *really* need some better checks via IsFrameOfType so we can get rid of the QI and avoid a lot of confusion. I'd like to see at least:
1) implements nsBlockFrame
2) IsFloatContainingBlock (which I think is the same as "lays out contents in block fashion", i.e. display-model:block (right?))
3) IsBlockLevel (i.e., acts like a block to the outside, i.e. display-role:block)

Comment 8

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

(In reply to comment #7)
> 3) IsBlockLevel (i.e., acts like a block to the outside, i.e.
> display-role:block)

I think this one is probably better determined from the style data than the frame type -- and we already can.  Lots of frame types (e.g., images, form controls) can be given display:block.  (One could argue that form controls should then become nsBlockFrames rather than form controls -- in fact, I think that probably would have been better -- but it's a little late for that now.)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, and currently IsFloatContainingBlock is true precisely for nsBlockFrame and its descendants. So maybe we just need one bit here, defined as both 1) and 2).

Comment 10

[Mats Palmgren (inactive)]

Note: if we decide to keep the QI's as is, we should probably make
nsComboboxControlFrame use nsHTMLContainerFrame too.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.394&root=/cvsroot&mark=321#291

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255726 [details] [diff] [review]
Patch rev. 1

Let's take this to fix the asserts and then we can work on eliminating uses of this CID for determining block-frame-ness.

Comment 12

[Boris Zbarsky [:bzbarsky]]

IsFloatContainingBlock() (or equivalent bit) should mean "won't drop frames that you put on its float list".  This is probably equivalent to "is an nsBlockFrame", but I'd also be happy using two different bits for those two slightly different concepts...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mats, do you want to check that in, or should I do it?

Comment 14

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-02-23 20:42 PST

-> FIXED

Comment 15

[Jesse Ruderman]

Should be covered by plenty of existing tests.  We just need assertions to turn Tinderbox orange!