731096 - OOM Crash [@ nsIFrame::GetStyleDisplay ] due to fallible allocation in nsFrameManager::RegisterPlaceholderFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Christian Holler (:decoder)]

The following source part (m-c revision 66e4d53697c2) indirectly uses fallible allocation through the nsFrameManager::RegisterPlaceholderFrame method (which returns an OOM error code):

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2907

If the call to nsFrameManager::RegisterPlaceholderFrame here fails, we'll crash:

#0  nsIFrame::GetStyleDisplay (this=<optimized out>) at /srv/repos/browser/mozilla-central/layout/generic/../style/nsStyleStructList.h:122
#1  0x00002aaaac6fc419 in nsIFrame::GetContainingBlock (this=0x0) at /srv/repos/browser/mozilla-central/layout/generic/nsFrame.cpp:5135
#2  0x00002aaaac71b7ee in nsHTMLReflowState::GetHypotheticalBoxContainer (this=0x7fffffffa190, aFrame=<optimized out>, aCBLeftEdge=@0x7fffffffa028, aCBWidth=@0x7fffffffa02c) at /srv/repos/browser/mozilla-central/layout/generic/nsHTMLReflowState.cpp:723
#3  0x00002aaaac71c470 in nsHTMLReflowState::InitAbsoluteConstraints (this=0x7fffffffa190, aPresContext=0x2ad9230, cbrs=0x7fffffffa778, containingBlockWidth=60, containingBlockHeight=60, aFrameType=0xc1d6b0) at /srv/repos/browser/mozilla-central/layout/generic/nsHTMLReflowState.cpp:1168
#4  0x00002aaaac71e386 in nsHTMLReflowState::InitConstraints (this=0x7fffffffa190, aPresContext=0x2ad9230, aContainingBlockWidth=60, aContainingBlockHeight=60, aBorder=<optimized out>, aPadding=<optimized out>, aFrameType=0xc1d6b0) at /srv/repos/browser/mozilla-central/layout/generic/nsHTMLReflowState.cpp:1863
#5  0x00002aaaac71e854 in nsHTMLReflowState::Init (this=0x7fffffffa190, aPresContext=0x2ad9230, aContainingBlockWidth=60, aContainingBlockHeight=60, aBorder=0x0, aPadding=0x0) at /srv/repos/browser/mozilla-central/layout/generic/nsHTMLReflowState.cpp:290
#6  0x00002aaaac6d85b5 in nsAbsoluteContainingBlock::ReflowAbsoluteFrame (this=<optimized out>, aDelegatingFrame=0x2bbc278, aPresContext=0x2ad9230, aReflowState=..., aContainingBlockWidth=60, aContainingBlockHeight=60, aConstrainHeight=false, aKidFrame=0x5a141b0, aStatus=@0x7fffffffa5cc, aOverflowAreas=0x7fffffffaae0) at /srv/repos/browser/mozilla-central/layout/generic/nsAbsoluteContainingBlock.cpp:422
#7  0x00002aaaac6d92e6 in nsAbsoluteContainingBlock::Reflow (this=0x3ffa6b0, aDelegatingFrame=0x2bbc278, aPresContext=0x2ad9230, aReflowState=..., aReflowStatus=@0x7fffffffabac, aContainingBlockWidth=60, aContainingBlockHeight=60, aConstrainHeight=false, aCBWidthChanged=true, aCBHeightChanged=true, aOverflowAreas=0x7fffffffaae0) at /srv/repos/browser/mozilla-central/layout/generic/nsAbsoluteContainingBlock.cpp:158


As the original fallible allocation is in NSPR which cannot be easily turned into infallible allocation, I propose to properly handle the OOM in nsCSSFrameConstructor.cpp instead.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is the allocation failure in the hashtable stuff?  If so, yes, the caller needs to cope with the failure.

Comment 2

[Christian Holler (:decoder)]

Yes, the first four frames of the failing allocation are this:

#0 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libmozalloc.so(moz_malloc+0x5f) [0x2aaaaab2415c] (aab2415c)
#1 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(+0x13e6224) (PL_DHashTableInit at /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/xpcom/build/pldhash.cpp:270)
#2 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(+0x828343) (nsFrameManager::RegisterPlaceholderFrame(nsPlaceholderFrame*) at /srv/repos/browser/mozilla-central/layout/base/nsFrameManager.cpp:289)
#3 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(+0x7f0d63) (nsCSSFrameConstructor::CreatePlaceholderFrameFor(nsIPresShell*, nsIContent*, nsIFrame*, nsStyleContext*, nsIFrame*, nsIFrame*, unsigned long, nsIFrame**) at /srv/repos/browser/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:2909)
#4 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(+0x7f1000) (nsFrameConstructorState::AddChild(nsIFrame*, nsFrameItems&, nsIContent*, nsStyleContext*, nsIFrame*, bool, bool, bool, bool, nsIFrame*) at /srv/repos/browser/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:1170)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Make nsFrameManager::RegisterPlaceholderFrame infallible.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b18d4cb81ea11382611fa7b92cd0906c9f61c3&exclusion_profile=false

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) from comment #1)
> Is the allocation failure in the hashtable stuff?  If so, yes, the caller
> needs to cope with the failure.

Following up on this bug comment from 5 years ago, for the record:
This changed slightly in bug 1131901 (where PL_DHashTableAdd() was made infallible by default). This is why RegisterPlaceholderFrame currently passes an explicit "fallible" keyword to this PLDHashTable "Add" API, and it's why we can make the allocation infallible by simply removing that keyword (in mats' patch from comment 3).

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8823050 [details] [diff] [review]
Make nsFrameManager::RegisterPlaceholderFrame infallible.

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

r=me

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537d72408280
Make nsFrameManager::RegisterPlaceholderFrame infallible.  r=dholbert

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/537d72408280

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Worth taking this on 52 for the next ESR? Seems like a pretty small patch?

Comment 9

[Mats Palmgren (inactive)]

Sure, this is a very safe patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8823050 [details] [diff] [review]
Make nsFrameManager::RegisterPlaceholderFrame infallible.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: OOM crashes in some situations
[Is this code covered by automated tests?]: layout is well-tested
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: just makes an allocation infallible
[String changes made/needed]: none

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8823050 [details] [diff] [review]
Make nsFrameManager::RegisterPlaceholderFrame infallible.

make an allocation infaillible, beta52+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7443e9844412