Closed Bug 852428 Opened 7 years ago Closed 7 years ago

Make nsIFrame::Init infallible

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

No description provided.
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.
I guess if we do this the question becomes whether it should be folded into the constructor too...
Blocks: 852501
Whiteboard: [need review]
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?
No, it can't.  I'll remove that comment.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
Attachment #726602 - Flags: review?(dholbert) → review+
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
> 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 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.
Attachment #726603 - Flags: review?(dholbert) → review+
> 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.
(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 on attachment 727010 [details] [diff] [review]
Interdiff addressing review comments

Interdiff looks good -- r=me with comment 13 addressed.
Attachment #727010 - Flags: review?(dholbert) → review+
> OK, cool -- could you add a comment stating that

Done.
You need to log in before you can comment on or make changes to this bug.