852428 - Make nsIFrame::Init infallible

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm going to assume that presshell arena stuff (e.g. style contexts) is infallible.  Though it's not clear to me that this is true in the DEBUG_TRACEMALLOC_PRESARENA case.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I guess if we do this the question becomes whether it should be folded into the constructor too...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make CreateViewForFrame infallible.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make nsIFrame::Init infallible.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 726603 [details] [diff] [review]
part 2.  Make nsIFrame::Init infallible.

Review of attachment 726603 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsImageControlFrame.cpp
@@ +113,3 @@
>  
>    // nsIntPoint allocation can fail, in which case we just set the property 
>    // to null, which is safe

Can it, though?

Comment 6

[Boris Zbarsky [:bzbarsky]]

No, it can't.  I'll remove that comment.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 726602 [details] [diff] [review]
part 1.  Make CreateViewForFrame infallible.

> nsView*
> nsViewManager::CreateView(const nsRect& aBounds,
>                           const nsView* aParent,
>                           nsViewVisibility aVisibilityFlag)
> {
[...]
>+  v->SetParent(const_cast<nsView*>(aParent));

Note: the "const" keyword on aParent is a bit of a sham here, and doesn't seem to even be needed. I filed bug 852742 on dropping it.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 726603 [details] [diff] [review]
part 2.  Make nsIFrame::Init infallible.

Feedback for the first half of "part 2" patch:
==============================================

>diff --git a/layout/forms/nsListControlFrame.h b/layout/forms/nsListControlFrame.h
>-  NS_IMETHOD Init(nsIContent*      aContent,
>-                   nsIFrame*        aParent,
>-                   nsIFrame*        aPrevInFlow);
>+  virtual void Init(nsIContent*      aContent,
>+                    nsIFrame*        aParent,
>+                    nsIFrame*        aPrevInFlow);

Mark this as MOZ_OVERRIDE, while you're here.

(All of the Init() impls touched by this change are marked as MOZ_OVERRIDE, as a sanity-check for patch correctness. Looks like you're already doing that elsewhere in this patch - nice.)

>diff --git a/layout/generic/nsFrameSetFrame.cpp b/layout/generic/nsFrameSetFrame.cpp
>@@ -276,57 +275,47 @@ nsHTMLFramesetFrame::Init(nsIContent*   
>-  nsresult result = ourContent->GetRowSpec(&mNumRows, &rowSpecs);
>-  NS_ENSURE_SUCCESS(result, result);
>-  result = ourContent->GetColSpec(&mNumCols, &colSpecs);
>-  NS_ENSURE_SUCCESS(result, result);
>+  ourContent->GetRowSpec(&mNumRows, &rowSpecs);
>+  ourContent->GetColSpec(&mNumCols, &colSpecs);

Hmm... GetRowSpec and GetColSpec are still fallible, but this removes error-handling for when they fail.

Will we still do the right thing when they fail?

>diff --git a/layout/generic/nsHTMLCanvasFrame.cpp b/layout/generic/nsHTMLCanvasFrame.cpp
>+void
> nsHTMLCanvasFrame::Init(nsIContent* aContent,
>                         nsIFrame*   aParent,
>                         nsIFrame*   aPrevInFlow)
> {
>-  nsresult rv = nsSplittableFrame::Init(aContent, aParent, aPrevInFlow);
>+  nsSplittableFrame::Init(aContent, aParent, aPrevInFlow);

erm.. Why does nsHTMLCanvasFrame invoke the nsSplittableFrame init specialization here, when it derives from nsContainerFrame (which in turn derives from nsSplittableFrame)?[1]

I'd expect this to be calling nsContainerFrame::Init.  This looks like a bug (possibly copypaste error from nsImageFrame), but probably worth fixing in a separate patch to keep this one targeted.  Mind filing a followup?

[1] https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLCanvasFrame.h#30

>--- a/layout/generic/nsIFrame.h
>+++ b/layout/generic/nsIFrame.h
>@@ -575,24 +575,22 @@ public:
>    *
>    * If the frame is a continuing frame, then aPrevInFlow indicates the previous
>    * frame (the frame that was split).
>    *
>    * If you want a view associated with your frame, you should create the view
>    * now.
>    *
>    * @param   aContent the content object associated with the frame
>-   * @param   aGeometricParent  the geometric parent frame
>-   * @param   aContentParent  the content parent frame
>-   * @param   aContext the style context associated with the frame
>+   * @param   aParent the parent frame
>    * @param   aPrevInFlow the prev-in-flow frame
>    */
>-  NS_IMETHOD  Init(nsIContent*      aContent,

While you're fixing up the documentation here -- maybe also clarify what "now" means, in "create the view now" means? (Presumably it means "after Init has returned")?

>diff --git a/layout/generic/nsTextFrame.h b/layout/generic/nsTextFrame.h
>--- a/layout/generic/nsTextFrame.h
>+++ b/layout/generic/nsTextFrame.h
>@@ -50,19 +50,19 @@ public:
>-  NS_IMETHOD Init(nsIContent*      aContent,
>-                  nsIFrame*        aParent,
>-                  nsIFrame*        aPrevInFlow);
>+  virtual void Init(nsIContent*      aContent,
>+                    nsIFrame*        aParent,
>+                    nsIFrame*        aPrevInFlow);

MOZ_OVERRIDE

>diff --git a/layout/svg/nsSVGForeignObjectFrame.h b/layout/svg/nsSVGForeignObjectFrame.h
>--- a/layout/svg/nsSVGForeignObjectFrame.h
>+++ b/layout/svg/nsSVGForeignObjectFrame.h
>@@ -26,19 +26,19 @@ class nsSVGForeignObjectFrame : public n
>   // nsIFrame:
>-  NS_IMETHOD  Init(nsIContent* aContent,
>-                   nsIFrame*   aParent,
>-                   nsIFrame*   aPrevInFlow);
>+  virtual void Init(nsIContent* aContent,
>+                    nsIFrame*   aParent,
>+                    nsIFrame*   aPrevInFlow);

MOZ_OVERRIDE

Comment 9

[Daniel Holbert [:dholbert]]

> Mark this as MOZ_OVERRIDE, while you're here.
> 
> (All of the Init() impls touched by this change are marked as MOZ_OVERRIDE,

Sorry, s/are marked/should be marked/

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 726603 [details] [diff] [review]
part 2.  Make nsIFrame::Init infallible.

>diff --git a/layout/svg/nsSVGTextPathFrame.h b/layout/svg/nsSVGTextPathFrame.h
>@@ -36,19 +36,19 @@ class nsSVGTextPathFrame : public nsSVGT
>+  virtual void Init(nsIContent*      aContent,
>+                    nsIFrame*        aParent,
>+                    nsIFrame*        aPrevInFlow);

MOZ_OVERRIDE

>diff --git a/layout/xul/base/src/nsBoxFrame.cpp b/layout/xul/base/src/nsBoxFrame.cpp
> nsBoxFrame::Init(nsIContent*      aContent,
>                  nsIFrame*        aParent,
>                  nsIFrame*        aPrevInFlow)
> {
[...]
>   // register access key
>-  rv = RegUnregAccessKey(true);
>-
>-  return rv;
>+  RegUnregAccessKey(true);

RegUnregAccessKey is techincally fallible, but it looks like it only fails if mContent is null (and I'm not immediately clear on when that would happen).

If we're guaranteed that mContent is true here (as it seems like we assume we are in other Init methods), then it's probably worth capturing RegUnregAccessKey's return value in a DebugOnly variable and asserting that it NS_SUCCEEDED.

>diff --git a/layout/xul/base/src/nsXULLabelFrame.cpp b/layout/xul/base/src/nsXULLabelFrame.cpp
>+void
> nsXULLabelFrame::Init(nsIContent*      aContent,
>                       nsIFrame*        aParent,
>                       nsIFrame*        aPrevInFlow)
> {
>-  nsresult rv = nsBlockFrame::Init(aContent, aParent, aPrevInFlow);
>-  if (NS_FAILED(rv))
>-    return rv;
>+  nsBlockFrame::Init(aContent, aParent, aPrevInFlow);
> 
>   // register access key
>-  return RegUnregAccessKey(true);
>+  RegUnregAccessKey(true);

Same here.

Tentatively marking as r+ w/ the above addressed, but it might be worth another brief review-cycle, depending on how you want to handle the fallible GetRowSpec, GetColSpec, and RegUnregAccessKey methods.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Will we still do the right thing when they fail?

I believe so, yes.  In particular, on failure they will set the array length to 0, so we'll act as if we had no specs there, which we handle fine.

> Mind filing a followup?

Bug 852803.

> maybe also clarify what "now" means

Done.

> MOZ_OVERRIDE

Done for nsTextFrame, nsSVGForeignObjectFrame, and nsSVGTextPathFrame.

> and I'm not immediately clear on when that would happen

Never.  The only frame with an null mContent is the viewport.

I just made RegUnRegAccessKey assert mContent and return void.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing review comments

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> > Will we still do the right thing when they fail?
> 
> I believe so, yes.  In particular, on failure they will set the array length
> to 0, so we'll act as if we had no specs there, which we handle fine.

OK, cool -- could you add a comment stating that, so it's clear that this code knows that those methods can fail, and is OK with their failure condition?

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 727010 [details] [diff] [review]
Interdiff addressing review comments

Interdiff looks good -- r=me with comment 13 addressed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> OK, cool -- could you add a comment stating that

Done.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushe:
           http://hg.mozilla.org/integration/mozilla-inbound/rev/c95e4dbd6daf
           http://hg.mozilla.org/integration/mozilla-inbound/rev/6a5bf6934ba8

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c95e4dbd6daf
https://hg.mozilla.org/mozilla-central/rev/6a5bf6934ba8