654369 - Remove OOM checks from layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Craig Topper]

Attachment: Part 1: Remove NS_NewCaret

Comment 2

[Craig Topper]

Attachment: Part 2: Remove oom check and outparam from PresShell::CloneStyleSet

Comment 3

[Craig Topper]

Attachment: Part 3: Make nsCSSRendering::Init return void

Comment 4

[Craig Topper]

Attachment: Part 4: Make nsContainerFrame::SetOverflowFrames return void

Comment 5

[Craig Topper]

Attachment: Part 5: Make nsFrameList::Init return void

Comment 6

[Craig Topper]

Attachment: Part 6: Remove OOM checks from nsImageMap

Comment 7

[Craig Topper]

Attachment: Part 7: Remove OOM checks from nsTextFrameThebes

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529631 [details] [diff] [review]
Part 1: Remove NS_NewCaret

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529632 [details] [diff] [review]
Part 2: Remove oom check and outparam from PresShell::CloneStyleSet

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529633 [details] [diff] [review]
Part 3: Make nsCSSRendering::Init return void

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529634 [details] [diff] [review]
Part 4: Make nsContainerFrame::SetOverflowFrames return void

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529635 [details] [diff] [review]
Part 5: Make nsFrameList::Init return void

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529643 [details] [diff] [review]
Part 6: Remove OOM checks from nsImageMap

This one I'm not convinced about; the page controls |cnt|, right?  These allocations would make more sense being fallible, perhaps.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 529644 [details] [diff] [review]
Part 7: Remove OOM checks from nsTextFrameThebes

r=me

Comment 15

[Craig Topper]

(In reply to comment #13)
> Comment on attachment 529643 [details] [diff] [review] [review]
> Part 6: Remove OOM checks from nsImageMap
> 
> This one I'm not convinced about; the page controls |cnt|, right?  These
> allocations would make more sense being fallible, perhaps.

Oops. I missed that that was actually a call to new[] rather than just new. I'll undo that.

Comment 16

[Boris Zbarsky [:bzbarsky]]

It's not just the new[]; the other allocation here are under content control too, I think.

We should have a followup bug to make that new[] fallible, at least.

Comment 17

[Craig Topper]

(In reply to comment #16)
> It's not just the new[]; the other allocation here are under content control
> too, I think.
> 
> We should have a followup bug to make that new[] fallible, at least.

The only other allocation is for the different kinds of Area. How can that one fail except in the case of the NOT_REACHED path. But since "area" isn't initialized at all before the switch there's no guarantee that it will be null on the NOT_REACHED path anyway. Maybe that was a bug in itself?

Comment 18

[Boris Zbarsky [:bzbarsky]]

My point is that the number of calls to AddArea is under content control.  We should probably make the |new| in AddArea fallible and just do nothing if it fails.

Comment 19

[Craig Topper]

Attachment: Part 8: Remove OOM checks from layout/generic

Comment 20

[Craig Topper]

Attachment: Part 8: Remove OOM checks from layout/generic

Comment 21

[Craig Topper]

Attachment: Part 8: Make nsLineIterator::Init return void

Comment 22

[Craig Topper]

Attachment: Part 9: Remove OOM checks from layout/generic

Comment 23

[Craig Topper]

Attachment: Part 10: Make nsFloatManager::StoreRegionFor return void

Comment 24

[Craig Topper]

Attachment: Part 11: Remove OOM checks from layout/base

Comment 25

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

Comment on attachment 530932 [details] [diff] [review]
Part 8: Make nsLineIterator::Init return void

>diff --git a/layout/generic/nsFloatManager.cpp b/layout/generic/nsFloatManager.cpp
>--- a/layout/generic/nsFloatManager.cpp
>+++ b/layout/generic/nsFloatManager.cpp
>@@ -570,17 +570,17 @@
>       mOld->List(stdout);
>     }
> #endif
> 
>     delete mNew;
>   }
> }
> 
>-nsresult
>+void
> nsAutoFloatManager::CreateFloatManager(nsPresContext *aPresContext)
> {
>   // Create a new float manager and install it in the reflow
>   // state. `Remember' the old float manager so we can restore it
>   // later.
>   mNew = new nsFloatManager(aPresContext->PresShell());
>   if (! mNew)
>     return NS_ERROR_OUT_OF_MEMORY;

You should remove that check in this patch, not in part 9.

Comment 26

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

Comment on attachment 530933 [details] [diff] [review]
Part 9: Remove OOM checks from layout/generic

>diff --git a/layout/generic/nsFrameUtil.cpp b/layout/generic/nsFrameUtil.cpp
>--- a/layout/generic/nsFrameUtil.cpp
>+++ b/layout/generic/nsFrameUtil.cpp
>@@ -503,31 +498,28 @@
>   if (type == openClose) {
>     aResult.Append(PRUnichar('/'));
>   }
>   aResult.Append(PRUnichar('>'));
> }
> 
> //----------------------------------------------------------------------
> 
>-nsresult NS_NewFrameUtil(nsIFrameUtil** aResult);
> nsresult
> NS_NewFrameUtil(nsIFrameUtil** aResult)
> {
>   NS_PRECONDITION(nsnull != aResult, "null pointer");
>   if (nsnull == aResult) {
>     return NS_ERROR_NULL_POINTER;
>   }
>-  *aResult = nsnull;
> 
>   nsFrameUtil* it = new nsFrameUtil();
>-  if (nsnull == it) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-  return it->QueryInterface(NS_GET_IID(nsIFrameUtil), (void**) aResult);
>+
>+  NS_ADDREF(*aResult = it);
>+  return NS_OK;
> }

This can just return nsFrameUtil* or already_AddRefed<nsFrameUtil>.

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>--- a/layout/generic/nsGfxScrollFrame.cpp
>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -1564,22 +1564,20 @@
> 
>   if (mAsyncScroll) {
>     if (mAsyncScroll->mIsSmoothScroll) {
>       currentPosition = mAsyncScroll->PositionAt(now);
>       currentVelocity = mAsyncScroll->VelocityAt(now);
>     }
>   } else {
>     mAsyncScroll = new AsyncScroll;
>-    if (mAsyncScroll) {
>-      mAsyncScroll->mScrollTimer = do_CreateInstance("@mozilla.org/timer;1");
>-      if (!mAsyncScroll->mScrollTimer) {
>-        delete mAsyncScroll;
>-        mAsyncScroll = nsnull;
>-      }
>+    mAsyncScroll->mScrollTimer = do_CreateInstance("@mozilla.org/timer;1");
>+    if (!mAsyncScroll->mScrollTimer) {
>+      delete mAsyncScroll;
>+      mAsyncScroll = nsnull;
>     }
>     if (!mAsyncScroll) {
>       // some allocation failed. Scroll the normal way.
>       ScrollToImpl(mDestination);
>       return;
>     }

I think this would read more easily as

    mAsyncScroll->mScrollTimer = do_CreateInstance("@mozilla.org/timer;1");
    if (!mAsyncScroll->mScrollTimer) {
      delete mAsyncScroll;
      mAsyncScroll = nsnull;
      ScrollToImpl(mDestination);
      return;
    }

>diff --git a/layout/generic/nsSelection.cpp b/layout/generic/nsSelection.cpp
>--- a/layout/generic/nsSelection.cpp
>+++ b/layout/generic/nsSelection.cpp
>@@ -501,28 +501,24 @@
>   PRUint32 mDelay;
> };
> 
> NS_IMPL_ISUPPORTS1(nsAutoScrollTimer, nsITimerCallback)
> 
> nsresult NS_NewSelection(nsFrameSelection **aFrameSelection)
> {
>   nsFrameSelection *rlist = new nsFrameSelection;
>-  if (!rlist)
>-    return NS_ERROR_OUT_OF_MEMORY;
>   *aFrameSelection = rlist;
>   NS_ADDREF(rlist);
>   return NS_OK;
> }
> 
> nsresult NS_NewDomSelection(nsISelection **aDomSelection)
> {
>   nsTypedSelection *rlist = new nsTypedSelection;
>-  if (!rlist)
>-    return NS_ERROR_OUT_OF_MEMORY;
>   *aDomSelection = (nsISelection *)rlist;
>   NS_ADDREF(rlist);
>   return NS_OK;
> }

These two can return the pointer as well.

>@@ -4745,17 +4726,17 @@
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsTypedSelection::GetEnumerator(nsIEnumerator **aIterator)
> {
>   nsresult status = NS_ERROR_OUT_OF_MEMORY;
>   nsSelectionIterator *iterator =  new nsSelectionIterator(this);
>-  if (iterator && NS_FAILED(status = CallQueryInterface(iterator, aIterator)) )
>+  if (NS_FAILED(status = CallQueryInterface(iterator, aIterator)) )
>     delete iterator;
>   return status;
> }

{
  NS_ADDREF(*aIterator = new nsSelectionIterator(this));
  return NS_OK;
}

Comment 27

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

Comment on attachment 530936 [details] [diff] [review]
Part 11: Remove OOM checks from layout/base

>diff --git a/layout/base/nsDocumentViewer.cpp b/layout/base/nsDocumentViewer.cpp
>--- a/layout/base/nsDocumentViewer.cpp
>+++ b/layout/base/nsDocumentViewer.cpp
>@@ -517,19 +517,16 @@
> // Class IDs
> static NS_DEFINE_CID(kViewManagerCID,       NS_VIEW_MANAGER_CID);
> 
> //------------------------------------------------------------------
> nsresult
> NS_NewDocumentViewer(nsIDocumentViewer** aResult)
> {
>   *aResult = new DocumentViewerImpl();
>-  if (!*aResult) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   NS_ADDREF(*aResult);
> 
>   return NS_OK;
> }

Can return a pointer.

>diff --git a/layout/base/nsFrameTraversal.cpp b/layout/base/nsFrameTraversal.cpp
>--- a/layout/base/nsFrameTraversal.cpp
>+++ b/layout/base/nsFrameTraversal.cpp
>@@ -146,18 +146,16 @@
> /************IMPLEMENTATIONS**************/
> 
> nsresult NS_CreateFrameTraversal(nsIFrameTraversal** aResult)
> {
>   NS_ENSURE_ARG_POINTER(aResult);
>   *aResult = nsnull;
> 
>   nsCOMPtr<nsIFrameTraversal> t(new nsFrameTraversal());
>-  if (!t)
>-    return NS_ERROR_OUT_OF_MEMORY;
> 
>   *aResult = t;
>   NS_ADDREF(*aResult);
> 
>   return NS_OK;
> }
> 
> nsresult
>@@ -174,18 +172,16 @@
>   nsCOMPtr<nsIFrameEnumerator> trav;
>   if (aVisual) {
>     trav = new nsVisualIterator(aPresContext, aStart, aType,
>                                 aLockInScrollView, aFollowOOFs);
>   } else {
>     trav = new nsFrameIterator(aPresContext, aStart, aType,
>                                aLockInScrollView, aFollowOOFs);
>   }
>-  if (!trav)
>-    return NS_ERROR_OUT_OF_MEMORY;
>   *aEnumerator = trav;
>   NS_ADDREF(trav);
>   return NS_OK;
> }

Same for these, but if you don't, can you please do
-   *aEnumerator = trav;
-   NS_ADDREF(trav);
+   trav.forget(aEnumerator);

>diff --git a/layout/base/nsLayoutDebugger.cpp b/layout/base/nsLayoutDebugger.cpp
>--- a/layout/base/nsLayoutDebugger.cpp
>+++ b/layout/base/nsLayoutDebugger.cpp
>@@ -79,19 +79,16 @@
> nsresult
> NS_NewLayoutDebugger(nsILayoutDebugger** aResult)
> {
>   NS_PRECONDITION(aResult, "null OUT ptr");
>   if (!aResult) {
>     return NS_ERROR_NULL_POINTER;
>   }
>   nsLayoutDebugger* it = new nsLayoutDebugger();
>-  if (!it) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>   return it->QueryInterface(NS_GET_IID(nsILayoutDebugger), (void**)aResult);
> }

Here, too.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -1631,18 +1625,16 @@
> NS_NewPresShell(nsIPresShell** aInstancePtrResult)
> {
>   NS_PRECONDITION(nsnull != aInstancePtrResult, "null ptr");
> 
>   if (!aInstancePtrResult)
>     return NS_ERROR_NULL_POINTER;
> 
>   *aInstancePtrResult = new PresShell();
>-  if (!*aInstancePtrResult)
>-    return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aInstancePtrResult);
>   return NS_OK;
> }

And here.

Of course, all of these can be done in followup bugs / patches and only apply if bz doesn't disagree.

Comment 28

[Craig Topper]

(In reply to comment #25)
> Comment on attachment 530932 [details] [diff] [review] [review]
> Part 8: Make nsLineIterator::Init return void
> 
> >diff --git a/layout/generic/nsFloatManager.cpp b/layout/generic/nsFloatManager.cpp
> >--- a/layout/generic/nsFloatManager.cpp
> >+++ b/layout/generic/nsFloatManager.cpp
> >@@ -570,17 +570,17 @@
> >       mOld->List(stdout);
> >     }
> > #endif
> > 
> >     delete mNew;
> >   }
> > }
> > 
> >-nsresult
> >+void
> > nsAutoFloatManager::CreateFloatManager(nsPresContext *aPresContext)
> > {
> >   // Create a new float manager and install it in the reflow
> >   // state. `Remember' the old float manager so we can restore it
> >   // later.
> >   mNew = new nsFloatManager(aPresContext->PresShell());
> >   if (! mNew)
> >     return NS_ERROR_OUT_OF_MEMORY;
> 
> You should remove that check in this patch, not in part 9.

Thanks. That must have happened when I fractured the patches and I didn't build with only a subset applied.

Comment 29

[Craig Topper]

Attachment: Part 8: Make nsLineIterator::Init return void

Comment 30

[Craig Topper]

Attachment: Part 8b: Make nsAutoFloatManager::CreateFloatManager return void

Comment 31

[Craig Topper]

Attachment: Part 9: Remove OOM checks from layout/generic

Comment 32

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

And because I like what you're doing so much:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#1934

Comment 33

[Craig Topper]

Attachment: Part 11: Remove OOM checks from layout/base

Comment 34

[Craig Topper]

(In reply to comment #32)
> And because I like what you're doing so much:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.
> cpp#1934

Aren't those all Arena allocations? Are those infallible?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Rulenode allocations are fallible, last I checked.  We can consider fixing that, but not in this bug!

Ms2ger, can I suggest not overquoting?  ;)

Craig, I'll try to get to these in the next 2 days.  Once we do these, I'd say push the patches and file a followup for any further work just to keep the bugs sane length.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530940 [details] [diff] [review]
Part 8: Make nsLineIterator::Init return void

Just nix init entirely in favor of constructor arguments.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530941 [details] [diff] [review]
Part 8b: Make nsAutoFloatManager::CreateFloatManager return void

r=me

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530934 [details] [diff] [review]
Part 10: Make nsFloatManager::StoreRegionFor return void

r=me

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530941 [details] [diff] [review]
Part 8b: Make nsAutoFloatManager::CreateFloatManager return void

Actually, sorry.  This operator new doesn't seem to be infallible.  Perhaps that should be changed.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530944 [details] [diff] [review]
Part 9: Remove OOM checks from layout/generic

The frameset frame code should use fallible allocations for all of that stuff since it's under content control.

The rest of the changes here look fine; maybe you want to split out the frameset changes into a separate patch...

Comment 41

[Boris Zbarsky [:bzbarsky]]

For that layout/base patch, there are some more new[] uses that may need to be fallible... Do you want me to review anyway?

Comment 42

[Craig Topper]

Attachment: Part 8: Remove nsLineIterator::Init by moving code to constructor

Comment 43

[Craig Topper]

Attachment: Part 9: Remove OOM checks from layout/generic

Comment 44

[Craig Topper]

Attachment: Part 11: Remove OOM checks from layout/base

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 536233 [details] [diff] [review]
Part 8: Remove nsLineIterator::Init by moving code to constructor

Hmm.  So I just realized that this is doing an allocation that should probably in fact be fallible.  In particular, mLines.....

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 536234 [details] [diff] [review]
Part 9: Remove OOM checks from layout/generic

r=me

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 536235 [details] [diff] [review]
Part 11: Remove OOM checks from layout/base

r=me

Comment 48

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/bf89d0e1a8d5
http://hg.mozilla.org/mozilla-central/rev/f8abcfe27753
http://hg.mozilla.org/mozilla-central/rev/eb26dea95622
http://hg.mozilla.org/mozilla-central/rev/2ff6a55908af
http://hg.mozilla.org/mozilla-central/rev/605c1bfa4ad2
http://hg.mozilla.org/mozilla-central/rev/acec89b9786b
http://hg.mozilla.org/mozilla-central/rev/8027b37a9b32

That would have been a good idea to rename the parts before pushing them to inbound because now, there are parts 2, 4, 5, 7, 10, 9 and 11 pushed in this order...

Comment 49

[Jesse Ruderman]

(In reply to comment #39)
> Comment on attachment 530941 [details] [diff] [review]
> Part 8b: Make nsAutoFloatManager::CreateFloatManager return void
> 
> Actually, sorry.  This operator new doesn't seem to be infallible.  Perhaps
> that should be changed.

What makes this particular operator new fallible?

Comment 50

[Boris Zbarsky [:bzbarsky]]

> What makes this particular operator new fallible?

It's a custom operator new that allocates memory via the presshell arena, not via global ::new.