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

RESOLVED FIXED in mozilla30

Status

()

Core
Layout: Tables
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8372850 [details] [diff] [review]
Use fallible allocations for storing the right and bottom border information for table cells
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
Blocks: 969864
(Assignee)

Updated

4 years ago
Attachment #8372850 - Flags: review?(roc)
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"?
(Assignee)

Comment 3

4 years ago
(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!
Flags: needinfo?(dbaron)
(Assignee)

Comment 4

4 years ago
dbaron: ping
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8372850 - Flags: review?(dbaron)
(Assignee)

Comment 8

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

Argh, sorry!
Attachment #8372850 - Attachment is obsolete: true
Attachment #8372850 - Flags: review?(dbaron)
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8378079 - Flags: review?(dbaron)
Attachment #8378079 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/6ecd75aa9821
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.