526853 - Firefox 3.6b1 & 3.7a1pre Crash [@ nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*, unsigned int*) ]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[chris hofmann]

spin off of [Bug 526587] new crashes as fall out of frame poisoning

reports at

http://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&query=nsCSSFrameConstructor%3A%3AMaybeRecreateContainerForFrameRemoval&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsCSSFrameConstructor%3A%3AMaybeRecreateContainerForFrameRemoval%28nsIFrame*%2C%20unsigned%20int*%29

stack looks like 

0  	xul.dll  	nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval  	 layout/base/nsCSSFrameConstructor.cpp:8955
1 	xul.dll 	nsCSSFrameConstructor::ContentRemoved 	layout/base/nsCSSFrameConstructor.cpp:7150
2 	xul.dll 	nsGenericElement::doRemoveChildAt 	content/base/src/nsGenericElement.cpp:3396
3 	xul.dll 	nsGenericElement::RemoveChildAt 	content/base/src/nsGenericElement.cpp:3324
4 	xul.dll 	nsGenericElement::doRemoveChild 	content/base/src/nsGenericElement.cpp:4042
5 	xul.dll 	nsGenericElement::RemoveChild 	content/base/src/nsGenericElement.cpp:3561
6 	xul.dll 	CreateElementTxn::UndoTransaction 	editor/libeditor/base/CreateElementTxn.cpp:191
7 	xul.dll 	EditAggregateTxn::UndoTransaction 	editor/libeditor/base/EditAggregateTxn.cpp:83
8 	xul.dll 	PlaceholderTxn::UndoTransaction 	editor/libeditor/base/PlaceholderTxn.cpp:95
9 	xul.dll 	nsTransactionItem::UndoTransaction 	editor/txmgr/src/nsTransactionItem.cpp:242
10 	xul.dll 	nsTransactionManager::UndoTransaction 	editor/txmgr/src/nsTransactionManager.cpp:201
11 	xul.dll 	nsEditor::Undo 	editor/libeditor/base/nsEditor.cpp:829
12 	xul.dll 	nsPlaintextEditor::Undo 	editor/libeditor/text/nsPlaintextEditor.cpp:1142
13 	xul.dll 	nsUndoCommand::DoCommand 	editor/libeditor/base/nsEditorCommands.cpp:90
14 	xul.dll 	nsControllerCommandTable::DoCommand 	embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191
15 	xul.dll 	nsBaseCommandController::DoCommand 	embedding/components/commandhandler/src/nsBaseCommandController.cpp:169
16 	xul.dll 	nsXBLPrototypeHandler::DispatchXBLCommand 	content/xbl/src/nsXBLPrototypeHandler.cpp:511
17 	xul.dll 	xul.dll@0x96b60b

Comment 2

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

Zack, can you look into this one?

Probably worth trying all those URLs just in case one can reproduce the bug.

Comment 3

[Zack Weinberg (:zwol)]

Unable to reproduce on any of the above, even with the exact same build cited in one of the crash reports, but it looks like it ought to be simple: what most of the URLs have in common is, unsurprisingly, a rich-text editor of some kind.  This *should* be a repro recipe, only it doesn't crash for me:

* Open up Gmail.
* Click "compose message".
* Type stuff, and add formatting to it.
* Undo an action, or select and replace some text, in a way that will cause nsGenericElement::RemoveChild to be called.

Probably you just need to be cleverer about your text manipulation.

(The exceptions to the "contains rich text editor" rule, among the above URLs, are 74.125.93.132/..., which is a Google cache of some Ecuadorean legal document; eu.levi.com, which is an online trouser store; and gigglehd.com, which is a computer parts store in some  language I don't have fonts for (possibly Korean, not sure).  None of them has a visible editor.  I don't know what the deal is there.)

Comment 4

[Zack Weinberg (:zwol)]

You know what?  This is another issue with the primary frame map.  nsCSSFrameConstructor::ContentRemoved() takes two nsIContent* pointers (labeled "container" and "child").  The first thing it does is look up the child in the primary frame map, and that's the frame it passes to MaybeRecreateContainerForFrameRemoval.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Exciting.

How about we land smaug's document cloning patch and then I refresh and land my patch to nuke the primary frame map altogether?  :(

I really can't say much more about this otherwise....  One thing that might help is running the steps we think _might_ trigger this in a debug build and seeing whether any asserts fire.  Preferably under XPCOM_DEBUG_BREAK=trap and a debugger. I just tried this on gmail, sadly, an get nothing. :(

Comment 7

[chris hofmann]

I guess we might be looking for three kinds of bugs.

1) click to crash or low user interaction
2) high volume fp crashes
and 3) real simple low risk fixes

if we put together several fixes for the third class of bugs thats just like fixing a top crash.

Comment 8

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

(In reply to comment #6)
> after we ship 3.6 we can revisit the other architectural issues that this bug
> might raise. 
> 
> sound like a plan?

Yes.

On 3.7 we will definitely land document cloning and nuke the primary frame map and this bug will either go away or morph into something quite different. We can't do that on 3.6.

Comment 9

[chris hofmann]

the volume on this continues to be low (0-5) crashes per day though oct/nov.

the limited number of url's we are getting are behind gmail, gdocs,  yahoo mail, and other auth or account systems. the musicaisccb, pcnetwork.ir and thaigaming sites seem to be the only chance at a public site were we could hit the crash.

   1 http://www.musicaisccb.com.br/chat/principal.php
   1 http://www.musicaisccb.com.br/chat/principal.php#
   2 http://www.pcnetwork.ir/newreply.php?do=newreply&noquote=1&p=169495
   1 http://www.pcnetwork.ir/newthread.php?do=newthread&f=244
   1 http://www.pcnetwork.ir/newthread.php?do=postthread&f=244
   1 http://www.thaigaming.com/editpost.php?do=editpost&postid=870650
   1 http://www.thaigaming.com/newthread.php?do=newthread&f=40

if the code review didn't turn up anything other than boris' ideas in commment 5 then we should leave this until after 3.6 ships.

the only otherthing to wait for are possibly better url's when testers start using beta4

Comment 10

[Jesse Ruderman]

INCO for 3.6, hopefully fixed by bug 500882 for 3.7.

Comment 11

[cmtalbert]

I'm seeing this on nightly.  It's very reproducible on nightly with windows 7, the latest flash player and this URL: http://www.webupd8.org/2010/12/how-to-test-ubuntu-1104-with-unity-in.html.

However, it's not reproducible if I save the page off.  That makes me suspect the flash ads on the page.

Tentatively reopening the bug, but I do realize this is hardly enough information to start generating a testcase, so feel free to close again if this doesn't help.

Tested on new profile with nightly(Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110511 Firefox/6.0a1) and  latest flash (10.3.181.14).  I've verified that it does *not* happen on linux and it also doesn't happen on windows with older versions of flash installed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Might be better to file a separate bug on that (which makes it clear that it's Flash 10.3 specific).

Comment 13

[Marcia Knous [:marcia]]

I filed Bug 656646 since I started seeing this in crash stats recently - specific to Mac but with the same URL.

Comment 14

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

I see the assertions in bug 656130 being triggered on this page. Let's see if that patch fixes it.

Comment 15

[(no longer active)]

You're talking about the URL in comment 11, right?

Comment 16

[Timothy Nikkel (:tnikkel)]

Yes. The page seems to do something different on Windows because I get those assertions on Windows but not Linux.

Comment 17

[Bob Clary [:bc] (inactive)]

see also bug 660451

Comment 18

[(no longer active)]

This should be fixed by bug 656130.