969883 - Remove some unneeded memory allocation failure handling code in table cell map code

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[(no longer active)]

It seems like the sizes for these data structures can be controlled from
Web content, and we are already prepared to deal with OOM conditions,
except that we are using infallible allocations by mistake.

Comment 1

[(no longer active)]

Attachment: Use fallible allocations for storing the right and bottom border information for table cells

Comment 2

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

Is the handling of allocation failure actually correct?  (I would expect ancient, untested, and poorly reviewed code here.  Remember that infallible allocation is a security measure in some sense.)  And in what sense is this controlled by Web content -- is it more than "more content leads to larger sizes"?

Comment 3

[(no longer active)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> Is the handling of allocation failure actually correct?  (I would expect
> ancient, untested, and poorly reviewed code here.  Remember that infallible
> allocation is a security measure in some sense.)

Good question.  A cursory look at nsCellMap.cpp seems to suggest that at least we don't crash if we get an allocation failure, but I have no idea whether we end up doing the "right thing" if we get an allocation failure here (and what the "right thing" would be here anyway.)

> And in what sense is this
> controlled by Web content -- is it more than "more content leads to larger
> sizes"?

Hmm, I guess so, which means that there is actually no good reason why we would want to do a fallible allocation here.

But the fact remains that this code is very old and I don't understand it much to feel safe about making a large amount of change here.  My immediate interest is to make sure that nsTArray::SetLength() returns void.  And to that goal, I would be willing to write an alternative patch which modifies nsTableCellMap::GetRightMostBorder and nsTableCellMap::GetBottomMostBorder to not check the return value of SetLength() and keep using nsTArray's here.  Looking at those two functions, such a patch would actually not change the semantics of the existing code in any way (since SetLength() will always return true now) so I would feel comfortable with that.  Larger changes to this code are not an attractive option though.

Please let me know how you'd like me to proceed.  Thanks!

Comment 4

[(no longer active)]

dbaron: ping

Comment 5

[(no longer active)]

dbaron: reping

I'd really like to land bug 969864 sooner before more of this bad pattern lands, and this is the last blocker for that.

Comment 6

[Mats Palmgren (inactive)]

FWIW, I think we should keep infallible allocation here.  Your suggestion for
GetRight/BottomMostBorder sounds fine to me.

Comment 7

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

I think these are content-controllable sizes (by setting large rowspan/colspan values), but we should just stick to nsTArray anyway, given that I think the out-of-memory handling looks very sketchy (having the cellmap in an inconsistent state seems bad to me, and any sign of ABORT0() basically means code that was poorly thought through).  If we ever see OOM crashes here show up, we can revisit.

Comment 8

[(no longer active)]

Comment on attachment 8372850 [details] [diff] [review]
Use fallible allocations for storing the right and bottom border information for table cells

Argh, sorry!

Comment 9

[(no longer active)]

Attachment: Remove some unneeded memory allocation failure handling code in table cell map code; r=dbaron

The data structures manipulated by this code use infallible memory
allocation, so the OOM handling code paths here are dead code.

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecd75aa9821

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ecd75aa9821

Comment 12

[Mats Palmgren (inactive)]

Changing the Summary since we didn't do what it currently says.