Not sure exactly who to give this to, so I'm cc'ing some layout heavy-hitters and assigning it to myself ... This bug was originally reported as bug 117141 (Composer crashes when loading pages with an HTML Select element [@FrameManager::GetPrimaryFrameFor]), for which I checked in a work around. The basic problem is that nsStyleChangeList::AppendChange() does not deal with the fact that anonymous frames can point to either an anonymous content node, or the content node for a parent frame. Should two or more sibling anonymous content frames require a frame change, we can get into trouble if we push an anonymous content node on the changelist, and then push the parent content node on the list, as the result of a sibling frame who's content pointer pointed to the parent content node. nsCSSFrameConstructor::ProcessRestyledFrames() processes the changelist backwards, which means it would process the parent content node first, so as a result, we end up blowing away the entire frame hierarchy including the anonymous content frames and all their anonymous content. Leaving deleted nodes in the change list that will be processed. When they do get processed we usually crash some place like GetPrimaryFrameFor(). I'll attatch a JS test case that demostrates this crash rather easily in the browser and viewer. It's probably a good idea to read over the comments dbaron and I made in bug 117141 for more details. A stack trace for the crash looks something like this: FrameManager::GetPrimaryFrameFor(FrameManager * const 0x03d4e118, nsIContent * 0x03ab4b68, nsIFrame * * 0x0012ccec) line 645 + 33 bytes PresShell::GetPrimaryFrameFor(const PresShell * const 0x03890b08, nsIContent * 0x03ab4b68, nsIFrame * * 0x0012ccec) line 5453 + 32 bytes nsCSSFrameConstructor::RecreateFramesForContent(nsIPresContext * 0x03951220, nsIContent * 0x03ab4b68, int 0, nsIStyleRule * 0x00000000, nsIStyleContext * 0x00000000) line 11834 nsCSSFrameConstructor::ProcessRestyledFrames(nsCSSFrameConstructor * const 0x03d53160, nsStyleChangeList & {...}, nsIPresContext * 0x03951220) line 10012 nsCSSFrameConstructor::AttributeChanged(nsCSSFrameConstructor * const 0x03d53160, nsIPresContext * 0x03951220, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int 3) line 10558 StyleSetImpl::AttributeChanged(StyleSetImpl * const 0x03622a30, nsIPresContext * 0x03951220, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 1495 PresShell::AttributeChanged(PresShell * const 0x03890b10, nsIDocument * 0x03cf6028, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 5121 + 61 bytes nsDocument::AttributeChanged(nsDocument * const 0x03cf6028, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 1970 + 36 bytes nsHTMLDocument::AttributeChanged(nsHTMLDocument * const 0x03cf6028, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 1463 nsGenericHTMLElement::SetAttr(nsGenericHTMLElement * const 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, const nsAString & {...}, int 1) line 1663 nsGenericHTMLElement::SetFormControlAttribute(nsIForm * 0x00000000, int 0, nsIAtom * 0x0158a710, const nsAString & {...}, int 1) line 4142 + 28 bytes nsGenericHTMLContainerFormElement::SetAttr(nsGenericHTMLContainerFormElement * const 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, const nsAString & {...}, int 1) line 4180 nsGenericHTMLElement::SetAttr(nsGenericHTMLElement * const 0x03c8fbb0, nsINodeInfo * 0x039519d8, const nsAString & {...}, int 1) line 1685 + 33 bytes nsGenericHTMLContainerFormElement::SetAttr(nsGenericHTMLContainerFormElement * const 0x03c8fbb0, nsINodeInfo * 0x039519d8, const nsAString & {...}, int 1) line 4188 nsGenericElement::SetAttribute(nsGenericElement * const 0x03c8fbb0, const nsAString & {...}, const nsAString & {...}) line 954 + 27 bytes nsGenericHTMLElement::SetAttribute(nsGenericHTMLElement * const 0x03c8fbb0, const nsAString & {...}, const nsAString & {...}) line 109 nsHTMLSelectElement::SetAttribute(nsHTMLSelectElement * const 0x03c8fbb0, const nsAString & {...}, const nsAString & {...}) line 140 + 20 bytes XPTC_InvokeByIndex(nsISupports * 0x03c8fbe0, unsigned int 30, unsigned int 2, nsXPTCVariant * 0x0012d5f8) line 106 XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_METHOD) line 1998 + 42 bytes XPC_WN_CallMethod(JSContext * 0x0361a768, JSObject * 0x02d18d40, unsigned int 2, long * 0x03cfad4c, long * 0x0012d8d4) line 1266 + 14 bytes js_Invoke(JSContext * 0x0361a768, unsigned int 2, unsigned int 0) line 832 + 23 bytes js_Interpret(JSContext * 0x0361a768, long * 0x0012e6c4) line 2800 + 15 bytes js_Invoke(JSContext * 0x0361a768, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x0361a768, JSObject * 0x02d17520, long 47287432, unsigned int 0, unsigned int 1, long * 0x0012e91c, long * 0x0012e7ec) line 924 + 20 bytes JS_CallFunctionValue(JSContext * 0x0361a768, JSObject * 0x02d17520, long 47287432, unsigned int 1, long * 0x0012e91c, long * 0x0012e7ec) line 3405 + 31 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x02fb10c8, void * 0x02d17520, void * 0x02d18c88, unsigned int 1, void * 0x0012e91c, int * 0x0012e920, int 0) line 1016 + 33 bytes nsJSEventListener::HandleEvent(nsJSEventListener * const 0x03d2bc00, nsIDOMEvent * 0x03c4fb70) line 180 + 77 bytes nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03949518, nsIDOMEvent * 0x03c4fb70, nsIDOMEventTarget * 0x0393ab70, unsigned int 4, unsigned int 7) line 1205 + 20 bytes nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03d2bb88, nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124, nsIDOMEventTarget * 0x0393ab70, unsigned int 7, nsEventStatus * 0x0012f828) line 1373 + 36 bytes nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x03d2b838, nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124, unsigned int 1, nsEventStatus * 0x0012f828) line 1652 nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x03d2b838, nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x00000000, unsigned int 1, nsEventStatus * 0x0012f828) line 1190 + 29 bytes PresShell::HandleEventInternal(nsEvent * 0x0012f48c, nsIView * 0x00000000, unsigned int 1, nsEventStatus * 0x0012f828) line 6000 + 44 bytes PresShell::HandleEventWithTarget(PresShell * const 0x03890b08, nsEvent * 0x0012f48c, nsIFrame * 0x03b5819c, nsIContent * 0x03d2b838, unsigned int 1, nsEventStatus * 0x0012f828) line 5969 + 22 bytes nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 0x0394a128, nsIPresContext * 0x03951220, nsMouseEvent * 0x0012f92c, nsEventStatus * 0x0012f828) line 2463 + 63 bytes nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x0394a130, nsIPresContext * 0x03951220, nsEvent * 0x0012f92c, nsIFrame * 0x03b5819c, nsEventStatus * 0x0012f828, nsIView * 0x0394da90) line 1544 + 28 bytes PresShell::HandleEventInternal(nsEvent * 0x0012f92c, nsIView * 0x0394da90, unsigned int 1, nsEventStatus * 0x0012f828) line 6020 + 43 bytes PresShell::HandleEvent(PresShell * const 0x03890b0c, nsIView * 0x0394da90, nsGUIEvent * 0x0012f92c, nsEventStatus * 0x0012f828, int 0, int & 1) line 5923 + 25 bytes nsView::HandleEvent(nsView * const 0x0394da90, nsGUIEvent * 0x0012f92c, unsigned int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 390 nsView::HandleEvent(nsView * const 0x03d06380, nsGUIEvent * 0x0012f92c, unsigned int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 347 nsView::HandleEvent(nsView * const 0x03d8f228, nsGUIEvent * 0x0012f92c, unsigned int 0, nsEventStatus * 0x0012f828, int 1, int & 1) line 347 nsViewManager::DispatchEvent(nsViewManager * const 0x03956ab8, nsGUIEvent * 0x0012f92c, nsEventStatus * 0x0012f828) line 1900 HandleEvent(nsGUIEvent * 0x0012f92c) line 83 nsWindow::DispatchEvent(nsWindow * const 0x0392c4fc, nsGUIEvent * 0x0012f92c, nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f92c) line 871 nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4527 + 21 bytes ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4779 nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 1507413, long * 0x0012fd20) line 3419 + 24 bytes nsWindow::WindowProc(HWND__ * 0x000b027e, unsigned int 514, unsigned int 0, long 1507413) line 1115 + 27 bytes USER32! 77e12e98() USER32! 77e130e0() USER32! 77e15824() nsAppShellService::Run(nsAppShellService * const 0x010d71a8) line 308 main1(int 1, char * * 0x003c7380, nsISupports * 0x00000000) line 1285 + 32 bytes main(int 1, char * * 0x003c7380) line 1625 + 37 bytes mainCRTStartup() line 338 + 17 bytes

Changing QA Contact

This bug rings a bell like in bug 79866. We fixed it over there by separation.

>nsCSSFrameConstructor::ProcessRestyledFrames() processes the changelist >backwards, which means it would process the parent content node first, so >as a result, we end up blowing away the entire frame hierarchy including >the anonymous content frames and all their anonymous content. Leaving >deleted nodes in the change list that will be processed. An alternative way (simple and very low-risk I might add) to reliably solve the problem could be to change ProcessRestyledFrames() to proceed as follows: 1. before kicking off: for each anonymous content frame, register a dummy property in the frame manager. 2. during processing, skip those (dead) anonymous frames for whom their property doesn't exist in the frame manager anymore. [The basic idea is that since frames notify the frame manager when they are going away, and the frame manager removes any property that has been registered for them, you would then tell if a frame is still alive or not by checking the property that you _know_ should have been there. And programmatically, the property can be an nsAutoVoidArray initialized with the frames, and for which the destroy function simply consists in removing the frame from the void array.]

Should this be fixed in 0.9.9? /be

This one is a topcrasher on M098, with a few incidents on the Trunk, so I think the answer to Brendan's previous question is "yes".

This bug is unlikely to be the topcrasher on the trunk. Please don't make this bug any more confusing than it already is. The topcrash on the trunk is likely related to the bug where OBJECT-replacement causes frames to be readded to the primary frame map during destruction, or a similar bug with block-in-inline stuff.

batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

I am unable to reproduce this crash on a 20021008 1.0 branch build. Do we have any recent Talkback data on this issue?

I was able to repro this on today's win2k trunk build.

Re-assigning to myself. Having seen tons of related bugs, I am going to attach patch that implements the idea I noted in comment 4. The patch is likely going to fix those numerous crasher bugs with "nsCSSFrameConstructor::ProcessRestyledFrames" in the stack trace (not 100% certain, didn't read all, please verify). Did a quick search in bugzilla and saw: bug 114513 N620 crash [@ 0x003b009c - nsCSSFrameConstructor::Proces... bug 124596 crashing Mozilla is child's play [HandleScrollEvent] bug 141590 M1BR crash [@0x00000000 - IsCanvasFrame][@ 0x032eb41c -ns... bug 166205 URL causes Mozilla to crash. Crash is severe, even XP's t... bug 187548 Editing or saving page with some given CSS crashes Mozilla bug 187671 {ib}crash in nsCSSFrameConstructor::StyleChangeReflow bug 187890 Crash scrolling page scottcollins.net - Trunk [@ IsCanvas... bug 133219 Browser crash when selecting alternate style sheet; M11A ... bug 166596 Crash saving page with border padding issues [@ nsStyleC... bug 123049 AppendChange() doesn't handle anonymous content frames, c... bug 154797 {ib}[RR]Browser crashes inserting linked stylesheet bug 166569 [RR/RCF]perhaps closing a window using a menu (or not usi...

Fixes this bug and related bugs that I can test so far. Note regarding the patch: the |if (nsLayoutAtoms::changeListProperty == aPropertyName)| isn't necessary since there is only one consumer at the moment. But it highlights that if people want to be notified in other cirumstances they can just register the same function and add their own |if| there.

Maybe I will change the name to: s/NotifyFrameDestroyed/RemoveFramePropertyCallback/

This patch does fix the crash in bug 187890, eg, but it also triggers the following assertion: ###!!! ASSERTION: unexpected null param: 'aPropertyName && aFrame', file /home/bzbarsky/mozilla/profile/mozilla/layout/html/base/src/nsFrameManager.cpp, line 2536 So the issue is that anonymous content frames can cause a reframe of the parent content node and don't stop the processing in ReResolveStyleContext as they should when this happens? Perhaps AppendChange should be smarter about screening our multiple changes targeted at the same content node?

Curious, I don't get the assertion. |aPropertyName| is always passed as a hard nsLayoutAtoms::changeListProperty. |aFrame| comes straight from the change list and so it can't be null, or... can it? Any clues? Might be other changes in your tree? Since the patch also fixes other issues (e.g., bug 154797, bug 154797, bug 187671, etc) where it is not clear where/when anonymous frames come in, seems that it is easier/simpler to avoid these headaches once for all. The root cause of the crashes are those frames in the change list that get destroyed due to some ripple effect along the way.

OK, found out, aFrame can be null in the change list: http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrameManager.cpp#1899 Need to update the patch with the appropriate null checks.

The cases where anonymous frames don't come in all have {ib} situations.... I'm looking into it right now.

Comment on attachment 110845 [details] [diff] [review] patch with null checks Migrating the earlier requests (bz, feel free to pick either since you are looking).

> where it is not clear where/when anonymous frames come in They do not. Those bugs are caused by the fact that reframing a frame that's part of an {ib} split actually reframes its _parent_. Thus any siblings of it that were in the change list end up dead... That issue should be fixed by moving {ib} handling to reflow. Out of curiousity, have you run any perf tests for silly things like changing .style.color 10000 times on something with your patch? Is there any perf impact?

I haven't done any perf measurements. I was mostly interested in the underlying logic that led to the crashers. There might be a little perf impact, but hopefully it is going to be noise compared to the titanic task that goes into ReResolve which populated the change list in the first place.

Ok... the more I think about this, the more convinced I get that we do actually want something like this patch. Trying to make sure that the changes are properly ordered in the list and don't stomp on each other seems like it would be too painful (code-readability-wise, and possibly perf-wise). One thing that concerns me in the use of the processQueue array is that it makes this code O(N^2) in the number of frames (due to the indexOf calls on every iteration). Would it be better, perhaps, to do the following? 1) go through and set the frame property on all frames in the list 2) go through and do whatever change is needed on all frames in the list with the property 3) go through and clear the frame property on all the frames. This should at least be linear in the number of changes (though possibly more expensive in the "average" case of only a few changes :( ). One other thing I'm wondering is why we process the change hints from the end forward... is there a good reason? If so, we should comment it. If not, would changing the order be better or not? (Obviously not a central issue for this bug).

bz, don't concern yourself that much with the perf impact in this situation because, practically, the size of the list is often O(1), i.e., in tens or less. Will be noise with todays's CPUs. Remember premature optimization? The cause of more harm than good, especially in this complex code. Let's instead whatch the Tp radars to see how they go. My gut feeling is that there won't be cause for another iteration that will obfuscate the code even more. (Not sure why the previous code walked the list backward. I have documented the reason of the backward walk that I added.)

rbs, the change list will include nearly everything on the page for things like stylesheet changes (to alternate sheets), stylesheet loads (once the css loader does not block the parser, these could come in after lots of layout is done), and skin switches (if we ever bring back dynamic skin switching). Even just the alternate stylesheet switching case worries me.... Could you try it on a few largish pages (eg http://www.w3.org/Style/CSS/) to see how it looks? But I agree that what I propose is not really that much better and is somewhat less clear... :(

Note that what I am meaning is that the cost of other things (reresolve/delete/reconstruct the frame tree) is often going to absorb the little extra in this case. Of course, optimization is generally a good thing. Browsing with a |printf| and going to the W3C CSS page, I see: processQueue count:1 processQueue count:1 processQueue count:1 processQueue count:1 processQueue count:1 processQueue count:1 processQueue count:1 processQueue count:105 <-- up tick when switching to an alternate style to processQueue count:1 destroy/reresolve/reconstruct. processQueue count:1 processQueue count:1 processQueue count:1

Actually, just thought that the queue could be turned to a hash if the lookup perf proves critical later on.

> That issue should be fixed by moving {ib} handling to reflow. I seem to remember that it used to be in reflow and kipp moved it to the frame construction code, so it isn't clear how effective/beneficial an u-turn will be. Got some handy bug numbers about the proposed change for future references? === Updating title to reflect the full extend of the problem (which includes anonymous content frames as well as non-anonymous ones). The patch isn't quite what is outlined in comment 4 since it is queueing the whole lot (that's why it fixes the other bugs).

bug 142585 and its dependencies... the code was moved to frame construction by waterson, and at this point pretty much everyone who has thought about it (including waterson) thinks that a u-turn is needed. Just search for bugs with "{ib}" in the summary... ;)

Saw it. Seems like a simpler/easier/better proposal for that {ib}.

Avoid |IndexOf()| with some voodoo. The patch capitalizes on the frame manager hashtable and uses |NS_IFRAME_MGR_REMOVE_PROP| to embed the cleanup along the way.

I thought about suggesting that... the problem is that the same frame can be in the list multiple times with several different hints, as far as I can tell (as long as none of those hints are nsChangeHint_ReconstructFrame)... Perhaps the code in nsStyleChangeList::AppendChange should just coalesce all hits, not just nsChangeHint_ReconstructFrame? That still makes the change list construction O(N^2), but would make for much cleaner arch elsewhere (eg we could use the "more efficient patch" as well as having a nice invariant about the change list....).

> the problem is that the same frame can be in > the list multiple times with several different hints, as far as I can tell Indeed, it seems that it was the price tag to keep AppendChange() linear. But since the poperty hashtable overwrites and only keep a single reference (the hint doesn't matter), it isn't strictly a "problem" per see. The earlier alternative to filter out these duplicates was that IndexOf :-) > Perhaps the code in nsStyleChangeList::AppendChange should just coalesce Yep, might need to redesign the data structure itself with a coalesce in mind to make it more efficient, e.g., sorted with respect to hints or something. Looks like some food for thoughts here.

> it isn't strictly a "problem" per see Yes, it is. If a frame is in the list with multiple hints, we will only process _one_ of the hints with that patch (for the other instances, the property will be gone).

Good catch... Need some extra voodoo, or perhaps a return to the safer IndexOf :-)

Comment on attachment 110845 [details] [diff] [review] patch with null checks OK, back to the safer IndexOf()... leaving the other patch until such a time (if any) where the change list is made to hold only unique entries. Premature optimzation? Beware.

Comment on attachment 110845 [details] [diff] [review] patch with null checks bz, r= for this crasher to let it bake in the tree a little bit? You have already looked at the patch in great detail. BTW, I could iterate on the "more efficient" patch to: 1) not set the option to remove the prop in the processing loop 2) then cleanup at the end, i.e., another loop over the change list with the option to now effectively remove the props that are still there. But I have been favoring keeping the code simpler in this round. What do you think?

Comment on attachment 110845 [details] [diff] [review] patch with null checks r=me, if there is no obvious perf impact on the alternate sheet switching thing I mentioned....

rbs, I know you were saying earlier not to worry too much about the perf impact of this, but I'm just curious if you've noticed any perf impact when loading a large doc (with tables and form elements) in composer? In composer, I believe the entire doc is loaded, and then some override stylesheets are applied.

I am not "perceiving" a significant perf impact -- there are the (usual) lags when opening the editor. I tried attachment 110876 [details] (warning: large table with forms, from bug 141572) ang got: [...] processQueue count: 1 processQueue count: 1 processQueue count: 1399 <-- uptick probably when override stylesheets kick in processQueue count: 180 processQueue count: 1 processQueue count: 1 [...] Since that testcase is already slow by itself, any extra thing is shadowed. However, since the perf issue has been recurring from bz and now yourself, you may be smelling something fishy. So I have iterated on the more efficient patch. Perhaps your apprehensions warrant switching to this one instead? [The bottom line is that it is necessary to be able to know when frames die along the way. So using markers such as I am doing seems inevitable (i.e., I can't think of another simpler, robust and secure way to address these crashers).]

Comment on attachment 111671 [details] [diff] [review] alternate high performance patch I think I like this more.... But we should really de-pseudoCOMify nsStyleChangeList (separate bug).

Comment on attachment 111671 [details] [diff] [review] alternate high performance patch sr=dbaron, although it might be good to add a comment briefly summarizing comment 31.

Checked in with updated comment. Watching Tinderbox didn't raise an audible alarm.

Something I missed.. the comment on the property in nsLayoutAtomList seems to have gotten lost (what type the prop is). Would you mind putting that back? (and r/sr=me on that change).

Checked in a comment to note down that it is |void*| -- it is a dummy property which just creates a null entry in the frame manager's hashtable. + frameManager->SetFrameProperty(changeData->mFrame, + nsLayoutAtoms::changeListProperty, nsnull, nsnull);

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