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.
Created attachment 8372850 [details] [diff] [review] Use fallible allocations for storing the right and bottom border information for table cells
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"?
(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!
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.
FWIW, I think we should keep infallible allocation here. Your suggestion for GetRight/BottomMostBorder sounds fine to me.
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 on attachment 8372850 [details] [diff] [review] Use fallible allocations for storing the right and bottom border information for table cells Argh, sorry!
Created attachment 8378079 [details] [diff] [review] 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.
Attachment #8378079 - Flags: review?(dbaron) → review+
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Changing the Summary since we didn't do what it currently says.
Summary: Use fallible allocations for storing the right and bottom border information for table cells → Remove some unneeded memory allocation failure handling code in table cell map code
You need to log in before you can comment on or make changes to this bug.