138725 - Trunk M1RC2 Crash going to this Harvard link [@ nsFormFrame::AddFormControlFrame]

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[greer]

I've been able to crash at will on the Trunk and the release candidate going to
these links:

http://jobs.boston.com/texis/js/+Zwwzm1wwwqFqm5w5ozmqwwwMFqtxd5BdDDzm1wwwdzmxww
wpzm-wwwBFqtxd5BdD/job.html?id=3ca887a44

http://jobs.boston.com/texis/js/+Zwwzm1wwwqFqm5w5ozmqww
wMFqtxd5BdDDzm1wwwdzmxwwwpzm-wwwBFqtxd5BdD/job.html?id=3c7a63b1

Which generate some significant recursion:

nsFormFrame::AddFormControlFrame
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 198]
nsFormFrame::AddFormControlFrame
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 166]
nsHTMLButtonControlFrame::SetInitialChildList
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsHTMLButtonControlFrame.cpp,
line 389]
nsCSSFrameConstructor::ConstructHTMLFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 4860]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7175]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessBlockChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13183]
nsCSSFrameConstructor::ConstructBlock
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13132]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 6320]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7214]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 11999]
nsCSSFrameConstructor::ConstructHTMLFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 4838]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7175]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 11999]
nsCSSFrameConstructor::ConstructTableCellFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2741]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 3005]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableRowFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2585]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2991]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableRowGroupFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2476]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2985]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2357]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 6380]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7214]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 11999]
nsCSSFrameConstructor::ConstructTableCellFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2741]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 3005]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableRowFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2585]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2991]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableRowGroupFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2476]
nsCSSFrameConstructor::TableProcessChild
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2985]
nsCSSFrameConstructor::TableProcessChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2899]
nsCSSFrameConstructor::ConstructTableFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 2357]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 6380]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7214]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessInlineChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13483]
nsCSSFrameConstructor::ConstructInline
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13264]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 6340]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7214]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ProcessBlockChildren
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13183]
nsCSSFrameConstructor::ConstructBlock
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13132]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 6320]
nsCSSFrameConstructor::ConstructFrameInternal
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7214]
nsCSSFrameConstructor::ConstructFrame
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 7063]
nsCSSFrameConstructor::ContentInserted
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 8768]
nsCSSFrameConstructor::ContentReplaced
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 8914]
nsCSSFrameConstructor::WipeContainingBlock
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 13650]
nsCSSFrameConstructor::ContentAppended
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp,
line 8256]
StyleSetImpl::ContentAppended
[d:\builds\seamonkey\mozilla\content\base\src\nsStyleSet.cpp, line 1500]
PresShell::ContentAppended
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5171]
nsDocument::ContentAppended
[d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 1897]
nsHTMLDocument::ContentAppended
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLDocument.cpp, line
1338]
HTMLContentSink::NotifyAppend
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp,
line 4826]
SinkContext::CloseContainer
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp,
line 1572]
HTMLContentSink::CloseContainer
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp,
line 3470]

Comment 1

[greer]

Nominating, since this one seems easy to reproduce.

Comment 2

[Matthias Versen [:Matti]]

see also bug 121591

Comment 3

[Andrew Schultz]

crashes on linux builds back to at least 20020114
OS->All

Comment 4

[Andrew Schultz]

Attachment: reduced testcase

if one set of <span> tags is removed, it loads ok, but crashes on exit.

this does look like bug 121591.

Comment 5

[Laura Krietemeyer]

Changing QA Contact

Comment 6

[Kevin McCluskey (gone)]

Reassigning to John

Comment 7

[Amarendra Hanumanula]

 Setting the priority to P2.

Comment 8

[Amarendra Hanumanula]

*** Bug 139221 has been marked as a duplicate of this bug. ***

Comment 9

[R.K.Aa.]

*** Bug 140057 has been marked as a duplicate of this bug. ***

Comment 10

[Kevin McCluskey (gone)]

nsbeta1+.

Comment 11

[John Keiser (jkeiser)]

OK, this has to do with the PFM not being updated for one reason or another. 
Specifically, GetPrimaryFrameFor in nsFormFrame::AddFormControlFrame() is
returning the old, destroyed primary frame.

I put some printfs in the constructors / destructors and here is what I see:

0x86d29ac::nsFormFrame()
0x86d2be8::nsHTMLButtonControlFrame
0x86d29ac::Destroy()
0x86d2be8::Destroy()
###!!! ASSERTION: frame was not removed from primary frame map before
destruction or was readded to map after being removed:
'!PL_DHASH_ENTRY_IS_BUSY(entry) || entry->frame != aFrame', file
nsFrameManager.cpp, line 1051
###!!! Break: at file nsFrameManager.cpp, line 1051
0x86d2be8::~nsHTMLButtonControlFrame
###!!! ASSERTION: frame was not removed from primary frame map before
destruction or was readded to map after being removed:
'!PL_DHASH_ENTRY_IS_BUSY(entry) || entry->frame != aFrame', file
nsFrameManager.cpp, line 1051
###!!! Break: at file nsFrameManager.cpp, line 1051
0x86d29ac::~nsFormFrame()
0x86d1004::nsFormFrame()
0x86d2be8::nsHTMLButtonControlFrame
0x86d2be8::SetFormFrame(0x86d29ac)
0x86d29ac::AddFormControlFrame(0x86d2c20)
0x86d1004::Destroy()
0x86d2be8::Destroy()
0x86d29ac::RemoveFormControlFrame(0x86d2c20)

Comment 12

[karnaze (gone)]

Attachment: untested patch which fixes the test case (but not the url)

This patch avoids reframing when an inline is appended to an inline block. It
fixes the test case but not the url.

Comment 13

[karnaze (gone)]

Taking the bug. I will be attaching a patch after some more testing.

Comment 14

[karnaze (gone)]

Attachment: patch to fix the bug

The patch (1) fixes a flaw in DoCleanupFrameReferences where a placeholder's
out of flow frame was not being used to recurse its children and (2) avoids
reframing when an inline is appended to the last inline frame of an inline that
contains a block. All of the regression tests passed except there were some
cases where frames were now not marked as being special, probably because of
not having to reframe them. If this is important, I guess the patch could be
changed to set the bit when the reframe is avoided.

Comment 15

[Chris Waterson]

The `sacred triad'? LOL!

WRT your changes to DoCleanupFrameReferences, it seems like we'll end up calling
DoCleanupFrameReferences twice on out-of-flow frames that have been constructed
in the current session. Specifically, WipeContainingBlock calls
CleanupFrameReferences for each of the frame ctor state's three lists
mAbsoluteItems, mFixedItems, and mFloatedItems. Does this matter? (It looks like
DoCleanupFrameReferences is idempotent, so my guess is `no, this won't matter'.)

Comment 16

[karnaze (gone)]

In WipeContainingBlock I tried calling CleanupFrameReferences on the floaters 
before the originals and there were still problems. So, although the patch may 
be calling CleanupFrameReferences twice for frames with placeholders, it doesn't 
seem to be a problem. I guess WipeContainingBlock doesn't even need to call 
CleanupFrameReferences any more for things with placeholder frames. I'm a bit 
afraid to remove that though, since I'm not sure how many tests cases we have.

Comment 17

[Alexandru Savulov]

Comment on attachment 81893 [details] [diff] [review]
patch to fix the bug

r= alexsavulov

i spoke with chris about this patch he wrote and based on his observations + 
watersons comment + i looked yesterday at the stack as john started to look at
the bug, yes, we need to do this.

Comment 18

[karnaze (gone)]

Attachment: alternate patch avoiding multiple calls to DoCleanupFrameReferences

This patch is similar to the previous one except that the calls to
CleanupFrameReferences have been removed for the absolute, fixed, and floater
lists (which should be taken care of in the modifications to
DoCleanupFrameReferences). This patch seems better, but is it?

Comment 19

[Chris Waterson]

sr=waterson on both patches.

I think the first version (attachment 81893 [details] [diff] [review]) ought to go on the branch:
DoCleanupFrameReferences seems to be idempotent so calling it more than once is
just a bit of a performance hit. (Add an XXX comment saying that we probably
_don't_ need to make the extra calls in WipeContainingBlock.)

Check the second version (attachment 81937 [details] [diff] [review]) in on the trunk, and we'll see if
we're right. :-)

Anyway, my $0.02.

Comment 20

[Chris Waterson]

Comment on attachment 81893 [details] [diff] [review]
patch to fix the bug

sr=waterson

Comment 21

[Chris Waterson]

Comment on attachment 81937 [details] [diff] [review]
alternate patch avoiding multiple calls to DoCleanupFrameReferences

sr=waterson

Comment 22

[Chris Waterson]

Adding back a keyword I whacked.

Comment 23

[Alexandru Savulov]

Comment on attachment 81937 [details] [diff] [review]
alternate patch avoiding multiple calls to DoCleanupFrameReferences

r= alexsavulov

cool ideea to apply the previous patch on the branch.

Comment 24

[Jaime Rodriguez, Jr.]

Has this landed on the trunk? It looks like a pretty sizeable patch, that
touches layout, so I'd like to verify the fix first, and make sure we don't
introduce any regressions.

Comment 25

[Jaime Rodriguez, Jr.]

Karnaze - Can you land this on the trunk today? We'd like to take this one, but,
we'd also like it to bake for a day or 2.

Comment 26

[R.K.Aa.]

i get a very similar stack testing url in bug 142198
http://www.internetwines.com/spirits.html

but top of stack (on a non-debug cvs, linux) is

#0 0x40138cb2 in nsVoidArray::InsertElementAt () from libxpcom.so
#1 0x417b2465 in nsFormFrame::AddFormControlFrame () from libgklayout.so
#2 0x417b21d7 in nsFormFrame::AddFormControlFrame () from libgklayout.so
#3 0x417bd0b8 in nsHTMLButtonControlFrame::SetInitialChildList () from
libgklayout.so

Comment 27

[R.K.Aa.]

*** Bug 142198 has been marked as a duplicate of this bug. ***

Comment 28

[karnaze (gone)]

FIXED_ON_TRUNK

Comment 29

[Jaime Rodriguez, Jr.]

amar - can you verify this fix on the trunk, and make sure it did not cause any
regressions?

we should look @ taking this topcrasher fix for RC2, if it is verified.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 81937 [details] [diff] [review]
alternate patch avoiding multiple calls to DoCleanupFrameReferences

a=asa (on behalf of drivers) for checkin to the 1.0 branch.

Comment 31

[greer]

Verified - Talkback shows no crashes in the Trunk builds following checkin. I
have tried all of the links and testcases with the 0509 build. No crash. 

Great job, karnaze, et. al.

Comment 32

[Jay Patel [:jay]]

Updating summary with M1RC2, since this is a topcrasher for Mozilla 1.0 RC2. 
Since we have verified this fix on the Trunk, it'll be a good idea to get this
checked onto the branch soon.

Comment 33

[scottputterman]

adding adt1.0.0+ for checkin to Mozilla 1.0 Branch.  Please get drivers approval
again before checking in since it's been more than 3 days since their previous
approval, and then add the fixed1.0.0 keyword.

Comment 34

[chris hofmann]

reapproval a=chofmann for the 1.0 branch for rc3.  let's get it in today.

Comment 35

[karnaze (gone)]

Maybe it would be better to check the 2nd patch into the branch, since it has 
been on the trunk for over a week now.

Comment 36

[karnaze (gone)]

Checked the alternate (2nd) patch into the m1.0 branch.

Comment 37

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

Bug 167915 comment 35 says that this caused bug 167915 (another crasher).

Comment 38

[karnaze (gone)]

Attachment: patch to negate alternate patch which caused bug 167915

The alternate patch was installed on the trunk and branch, but it caused bug
167915. This patch undoes it and installs the 1st patch. This patch is also an
attachment in bug 167915.

Comment 39

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2