250493 - nsHTMLFramesetFrame::Init doesn't check for failure

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[timeless]

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: first patch

ok. first try.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: second try

a few fixes and one good feature - it compiles now.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: third

third. Addresing biesi's notices.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164137 [details] [diff] [review]
third

It'll take me a bit of time to get to this... In the meantime, do frames
document whether callers need to Destroy() when Init() fails?  Have you checked
whether the caller of Init() here even checks the return value?

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

no it's not defined.
I based on
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#630 

Comment 6

[Boris Zbarsky [:bzbarsky]]

I wouldn't base this sort of thing on anything else in layout... chances are,
the thing you're basing on is wrong.  ;)

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: fixed

Ouch!
There was a mistake in if (!mHorBorders) - fixed.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also i'd like to notice that this patch for some reason breaks
http://www.24fun.com/downloadcenter/benchjs/benchjs.html because of this checks:

       } else { // frame
         result = NS_NewSubDocumentFrame(shell, &frame);
-        frame->Init(aPresContext, child, this, kidSC, nsnull);
+        if (!result)
+          return NS_ERROR_OUT_OF_MEMORY;
+
+        result = frame->Init(aPresContext, child, this, kidSC, nsnull);
+        if (NS_FAILED(result)) {
+          frame->Destroy(aPresContext);
+          return result;
+        }

it looks that result = NS_NewSubDocumentFrame(shell, &frame); fails on this
site. I can predict it's a NS_NewSubDocumentFrame fault and fill a bug for it,
but i'm not sure...

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: better patch

Ok. This fixes the bug i mentioned above.
Now we check if NS_FAILED(result) rather than if !result.

Thanks biesi

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164395 [details] [diff] [review]
better patch

For future reference, more context (at least -u6, possibly more depending on
the code) usually makes patches easier to review...

>Index: nsFrameSetFrame.cpp
>   nsresult result = CallCreateInstance(kViewCID, &view);
>+  if (NS_FAILED(result))
>+    return result;
>+

Note that this needs to be merged to trunk (which now error-checks view
creation).

>   mChildTypes = new PRInt32[numCells]; 
>   mChildFrameborder  = new nsFrameborder[numCells]; 
>   mChildBorderColors  = new nsBorderColor[numCells]; 
>+  if (!mChildTypes || !mChildFrameborder || !mChildBorderColors)
>+    return NS_ERROR_OUT_OF_MEMORY;

This leaks if some of those things got allocated...  Please add code to the
destructor to delete them, and set them to null where they are currently
deleted (in reflow).

>@@ -397,18 +414,32 @@ nsHTMLFramesetFrame::Init(nsPresContext*

>         mChildBorderColors[mChildCount].Set(childFrame->GetBorderColor());
>+

Don't add that blank line, please.

>@@ -435,18 +466,24 @@ nsHTMLFramesetFrame::Init(nsPresContext*
>     pseudoStyleContext = shell->StyleSet()->ResolvePseudoStyleFor(nsnull,
>                                                                   nsCSSAnonBoxes::framesetBlank,
>                                                                   mStyleContext);
>+    if (!pseudoStyleContext)
>+      return NS_ERROR_OUT_OF_MEMORY;                                                                  

Don't you need to deallocate blankFrame here?

>       blankFrame->Init(aPresContext, mContent, this, pseudoStyleContext, nsnull);

And error-check that?

>-    
>+

Please don't make random whitespace changes like that (there are a few others
in this patch too); they just make the cvs blame harder to read.

r- because of the leak issue and the blank frame issues... sorry this took so
long; the next iteration will be much much faster, I promise.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: fixing issues from Comment 10

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: removing some comment lines from patch

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 166987 [details] [diff] [review]
removing some comment lines from patch

sorry for spam

Comment 14

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

Comment on attachment 166987 [details] [diff] [review]
removing some comment lines from patch

>@@ -1209,6 +1246,10 @@
>       }
>     }
> 
>+    mChildTypes = null;
>+    mChildFrameborder = null;
>+    mChildBorderColors = null;
>+
>     delete[] verBordersVis;    
>     delete[] verBorderColors;
>     delete[] horBordersVis; 

This is pretty obviously wrong, since you're assigning null to something that's
delete[]d just 10 lines below.	Either the delete[]s should be removed or these
belong somewhere else (such as *after* the delete[]s).

Comment 15

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

(Judging from comment 10, what bz intended to add those assignments to null
*after* the deletes.)

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: fixes bug mentioned in Comment 14

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167891 [details] [diff] [review]
fixes bug mentioned in Comment 14

>@@ -395,18 +408,31 @@ >nsHTMLFramesetFrame::Init(nsPresContext*
>       kidSC = shell->StyleSet()->ResolveStyleFor(child, mStyleContext);
 >	if (tag == nsHTMLAtoms::frameset) {
>         result = NS_NewHTMLFramesetFrame(shell, &frame);
>-
>+        if (NS_FAILED(result))
>+          return result;
>+    

Please don't add a space on that blank line.

r+sr=bzbarsky with that one char removed.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: final patch

removed white spaces in blank lines. taking r/sr from previous.

Comment 19

[Vidar Haarr (not reading bugmail)]

gandalf: check this in?

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in layout/generic/nsFrameSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrameSetFrame.cpp,v  <--  nsFrameSetFrame.cpp
new revision: 3.157; previous revision: 3.156
done