620275 - remove inconsistent null check of aContent from nsSubDocumentFrame::Init

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[timeless]

161 nsSubDocumentFrame::Init(nsIContent*     aContent,
162                          nsIFrame*       aParent,
163                          nsIFrame*       aPrevInFlow)
164 {
i'm not sure you actually need an if here anyway, since do_QI is null safe, and perhaps you want to pick a value if !aContent since you're Init()ing something...
165   // determine if we are a <frame> or <iframe>
166   if (aContent) {
167     nsCOMPtr<nsIDOMHTMLFrameElement> frameElem = do_QueryInterface(aContent);
168     mIsInline = frameElem ? PR_FALSE : PR_TRUE;
169   }

this is actually null safe:
171   nsresult rv =  nsLeafFrame::Init(aContent, aParent, aPrevInFlow);

we'll crash here:
191   aContent->SetPrimaryFrame(this);

Comment 1

[timeless]

Attachment: proposal

Comment 2

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

Comment on attachment 498679 [details] [diff] [review]
proposal

I don't think it's valid to create any frame other than the root frame with a null content node (or *maybe* part of the root scrollframe construction).  I think the right thing here is to remove the other null-check, but bzbarsky is probably a better reviewer than I am.

Comment 3

[timeless]

Attachment: alternative

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 498909 [details] [diff] [review]
alternative

That null-check is bogus.  Just remove it.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 498909 [details] [diff] [review]
alternative

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 498909 [details] [diff] [review]
> alternative
> 
> That null-check is bogus.  Just remove it.


This patch looks fine to me (with a little unbitrotting applied), so I'm
not sure which null-check you were referring to.  The 'frameElem'
null-check is still needed to check if it's a <frame> or <iframe>
as the comment says.  I don't see any other null-checks.

Comment 6

[Mats Palmgren (inactive)]

Attachment: unbitrotted

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, I have no idea what I was thinking then, sorry.  I agree that the patch looks right.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e122c5300afc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e122c5300afc