492014 - "ASSERTION: aContent2 must not be null" with caption, td, iframe

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: aContent2 must not be null: 'aContent2', file /Users/jruderman/central/layout/base/nsLayoutUtils.cpp, line 344

###!!! ASSERTION: aContent1 must not be null: 'aContent1', file /Users/jruderman/central/layout/base/nsLayoutUtils.cpp, line 343

Comment 1

[Jesse Ruderman]

Attachment: stack trace for one of the assertions

Comment 2

[Boris Zbarsky [:bzbarsky]]

In IsContentLEQ we have:

429       return nsLayoutUtils::CompareTreePosition(
430           aItem1->GetUnderlyingFrame()->GetContent(),
431           aItem2->GetUnderlyingFrame()->GetContent(),
432           static_cast<nsIContent*>(aClosure)) <= 0;

aItem2->GetUnderlyingFrame() is the viewport in this case, so it has no content. aItem2 is an nsDisplaySolidColor*, added in bug 485275.  aItem1 is the iframe's border item.

This doesn't look like a tables issue except insofar as nsTableOuterFrame::BuildDisplayList calls SortAllByContentOrder on the list it builds (and happens to be the only current consumer of said API).  That said, it doesn't make much sense to sort by content order a list that might include content from different documents (it certainly includes frames from different documents!)...  roc, any idea how that's expecting to work here?

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to comment #2)
> That said,
> it doesn't make much sense to sort by content order a list that might include
> content from different documents (it certainly includes frames from different
> documents!)...  roc, any idea how that's expecting to work here?

See bug 490376, comment 5.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  Well, maybe we should just fix that here.  It'd certainly resolve this bug!

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Here is a patch doing that.

I added the testcase as a crashtest even though it doesn't crash, perhaps we will start checking assertions. Is there a better way to test this?

Maybe give this a spin on try-server to be on the safe side?

Comment 6

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

Comment on attachment 376539 [details] [diff] [review]
patch

+  if (f)
+    aBuilder->LeavePresShell(f, dirty);

{} around this statement.

Thanks!

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 8

[Boris Zbarsky [:bzbarsky]]

We plan to start doing fatal asserts during crashtests, yes.

Comment 9

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

(In reply to comment #8)
> We plan to start doing fatal asserts during crashtests, yes.

No, we plan to make unexpected assertions during crashtests cause a test failure but not prevent the rest of the tests from running, and we plan to have a way to mark known assertions so we can get the numbers down.  (That does leave room for assertions to regress between now and when we get debug unit test tinderboxes running, but once it's running we'll have a mechanism for pushing the numbers down.)

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3 (needs to be applied after the patch in bug 491848)

The patch for this bug will conflict with the patch for bug 491848. So here is a patch that can be applied after bug 491848.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: patch v4 [Checkin: Comment 12]

This bug no longer has any dependency on bug 491848 so I figured I'd refresh the patch so there was no confusion.

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/9aac3ff92a06

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.1 patch v4 [Checkin: Comment 14]

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 377788 [details] [diff] [review]
1.9.1 patch v4
[Checkin: Comment 14]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/560b588c0ae8

after fixing context for
{
patching file layout/base/crashtests/crashtests.list
Hunk #1 FAILED at 135
1 out of 1 hunks FAILED
}

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on debug builds(no crash or assert):

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre ID:20090528130303

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre ID:20090528112613