User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20051008 Firefox/1.0.7 (Marc) Build Identifier: Tracker for work on deCOMtaminating the NS_New*Frame functions. Reproducible: Always Steps to Reproduce: This is the list of functions I found: NS_NewAbsoluteItemWrapperFrame NS_NewAreaFrame NS_NewBRFrame NS_NewBlockFrame NS_NewBoxFrame NS_NewButtonBoxFrame NS_NewCanvasFrame NS_NewColumnSetFrame NS_NewComboboxControlFrame NS_NewCommentFrame NS_NewContinuingTextFrame NS_NewDeckFrame NS_NewDocumentElementFrame NS_NewEmptyFrame NS_NewFieldSetFrame NS_NewFileControlFrame NS_NewFirstLetterFrame NS_NewFirstLineFrame NS_NewFloatingItemWrapperFrame NS_NewGfxAutoTextControlFrame NS_NewGfxButtonControlFrame NS_NewGfxCheckboxControlFrame NS_NewGfxRadioControlFrame NS_NewGridRowGroupFrame NS_NewGridRowLeafFrame NS_NewGrippyFrame NS_NewHTMLButtonControlFrame NS_NewHTMLCanvasFrame NS_NewHTMLFramesetFrame NS_NewHTMLScrollFrame NS_NewImageBoxFrame NS_NewImageControlFrame NS_NewImageFrame NS_NewInlineFrame NS_NewIsIndexFrame NS_NewLeafBoxFrame NS_NewLegendFrame NS_NewLineBox NS_NewListBoxBodyFrame NS_NewListControlFrame NS_NewListItemFrame NS_NewMathMLForeignFrameWrapper NS_NewMathMLTokenFrame NS_NewMathMLmactionFrame NS_NewMathMLmathBlockFrame NS_NewMathMLmathFrame NS_NewMathMLmathInlineFrame NS_NewMathMLmfencedFrame NS_NewMathMLmfracFrame NS_NewMathMLmmultiscriptsFrame NS_NewMathMLmoFrame NS_NewMathMLmoverFrame NS_NewMathMLmpaddedFrame NS_NewMathMLmphantomFrame NS_NewMathMLmrootFrame NS_NewMathMLmrowFrame NS_NewMathMLmsFrame NS_NewMathMLmspaceFrame NS_NewMathMLmsqrtFrame NS_NewMathMLmstyleFrame NS_NewMathMLmsubFrame NS_NewMathMLmsubsupFrame NS_NewMathMLmsupFrame NS_NewMathMLmtableFrame NS_NewMathMLmtableOuterFrame NS_NewMathMLmtdFrame NS_NewMathMLmtdInnerFrame NS_NewMathMLmtrFrame NS_NewMathMLmunderFrame NS_NewMathMLmunderoverFrame NS_NewMenuBarFrame NS_NewMenuFrame NS_NewMenuPopupFrame NS_NewNativeButtonControlFrame NS_NewNativeCheckboxControlFrame NS_NewNativeRadioControlFrame NS_NewNativeScrollbarFrame NS_NewNativeSelectControlFrame NS_NewNativeTextControlFrame NS_NewObjectFrame NS_NewPageBreakFrame NS_NewPageContentFrame NS_NewPageFrame NS_NewPlaceholderFrame NS_NewPopupSetFrame NS_NewPositionedInlineFrame NS_NewProgressMeterFrame NS_NewRelativeItemWrapperFrame NS_NewResizerFrame NS_NewSVGClipPathFrame NS_NewSVGDefsFrame NS_NewSVGGFrame NS_NewSVGGenericContainerFrame NS_NewSVGLinearGradientFrame NS_NewSVGMarkerFrame NS_NewSVGRadialGradientFrame NS_NewSVGTSpanFrame NS_NewScrollBarButtonFrame NS_NewScrollbarFrame NS_NewSelectsAreaFrame NS_NewSimplePageSequenceFrame NS_NewSliderFrame NS_NewSpacerFrame NS_NewSplitterFrame NS_NewStackFrame NS_NewSubDocumentFrame NS_NewTableCaptionFrame NS_NewTableCellFrame NS_NewTableCellInnerFrame NS_NewTableColFrame NS_NewTableColGroupFrame NS_NewTableFrame NS_NewTableOuterFrame NS_NewTableRowFrame NS_NewTableRowGroupFrame NS_NewTextBoxFrame NS_NewTextControlFrame NS_NewTextFrame NS_NewTitleBarFrame NS_NewTreeBodyFrame NS_NewTreeColFrame NS_NewViewportFrame NS_NewWBRFrame NS_NewXULScrollFrame

First chunk

nsAreaFrame* it = new (aPresShell) nsAreaFrame; - if (nsnull == it) { - return NS_ERROR_OUT_OF_MEMORY; - } it->SetFlags(aFlags); You'll want to return before using 'it' if it's null. + newFrame = NS_NewAreaFrame(mPresShell, NS_BLOCK_SPACE_MGR | NS_BLOCK_SHRINK_WRAP | NS_BLOCK_MARGIN_ROOT); + + if (NS_UNLIKELY(!newFrame)) { + rv = NS_ERROR_OUT_OF_MEMORY; + } Here, and elsewhere, you may be able to move the check of !newFrame down and using a single check to check lots of different allocation sites.

> Here, and elsewhere, you may be able to move the check of !newFrame down and > using a single check to check lots of different allocation sites. If I do that then I'll need to put a bigger number of *Frame s in an individual patch. Would you rather I did more at once?

Yes, it's probably best to do it all at once. Thanks for taking this on.

This is every NS_New*Frame used in nsCSSFrameConstructor::ConstructXULFrame along with some other odd ones and all of their callers and dependencies. The patch is already ~2000 lines so I think it'd be better to finish this and get it checked in before moving onto the rest.

Comment on attachment 200881 [details] [diff] [review] All of ConstructXULFrame Looks great! The //xxx marc comments I assume are we you can consolidate 'if' tests after you've done the other frame types. I'll check this in.

checked in. BTW you've been marking your patches r+ when you really should be marking them r? roc@ocallahan.org

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

This is all of ConstructHTMLFrame, callers and dependencies. Only 104 more to go if this patch is OK.

Comment on attachment 201207 [details] [diff] [review] All of ConstructHTMLFrame remember to request review from someone specific or your patch might get lost!

Comment on attachment 201207 [details] [diff] [review] All of ConstructHTMLFrame great!

Checked that in, thanks

I don't know why NS_NewDirectionalFrame doesn't allocate using the presshell. If you can't see any reason to not use the presshell, can you please change it so that it does? Would you mind deCOMtaminating the nsTableCreator::Create functions too? #if 0 - // set the state flags (if any are provided) - it->AddStateBits(NS_BLOCK_SPACE_MGR); + // set the state flags (if any are provided) + it->AddStateBits(NS_BLOCK_SPACE_MGR); #endif You can just remove this completely

Done those 3 things. Only SVG, most of MathML and 3ish XTF Frames to go. 53 in total.

Comment on attachment 201686 [details] [diff] [review] Near-all non-SVG/non-MathML NS_NewFrames 2 (with table Creates) lovely

checked that in

Could this be causing some trunk crashes I've been experiencing lately? http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB11431671M

(In reply to comment #18) > Could this be causing some trunk crashes I've been experiencing lately? > > http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB11431671M I filed bug 315127 on the crash.

This is all of the MathML and the 3 XTF NS_NewFrames. About the SVG ones. Some of the NS_New functions can return an error for their result without it meaning OOM. It would be possible to deCOM some of them but the resulting code would probably not be much of an improvement. Should I just leave all of them as they are or would it be better to try and find a solution with automatically creating a GenericContainer and returning it in the right places?

I've submitted a partial backout of bug 201686 in bug 315127. No big deal although we should have caught that crash in testing. (In reply to comment #20) > About the SVG ones. Some of the NS_New functions can return an error for their > result without it meaning OOM. It would be possible to deCOM some of them but > the resulting code would probably not be much of an improvement. > > Should I just leave all of them as they are or would it be better to try and > find a solution with automatically creating a GenericContainer and returning > it in the right places? No, the generic container fallback should stay where it is in nsCSSFrameConstructor. The caller is treating all failure results the same way and just ignoring the different return codes. I think you should go ahead and use nsnull to represent all failures.

This is all of the remaining NewFrame functions. For the SVG functions the frame will be null after OOM and after the QI failure. After this all NS_NewFrames return their frame directly.

Comment on attachment 202097 [details] [diff] [review] All of mathml and svg frames looks good. I'll land it.

landed. I guess this is closed now? Thanks so much! Any thoughts on what you might do next?

Should this now be RESOLVED FIXED?

yep

Is there a reason this patch didn't add |triedFrame = PR_TRUE;| to the HTML <input> case?

Please ignore comment 27 -- I figured it out....