969879 - Use fallible allocation to store the style information for CSS transitions and animations

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, 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 allocation to store the style information for CSS transitions and animations

Comment 2

[Daniel Holbert [:dholbert]]

Seems like we can't do this unless the relevant code that modifies these arrays actually checks for whether allocation fails. (and that we behave sanely after that).

Is that the case?

(Side note: a while back, we made an intentional choice to use infallible allocators in the CSS parser, in bug 659963. That doesn't necessarily mean we have to be infallible here as well, though.)

Comment 3

[Daniel Holbert [:dholbert]]

Also: at some point in the past, I remember being made aware of a rule-of-thumb that we should use fallible allocation when *both* of the following hold:
  (a) data's size is author-controllable
  (b) the data's size can be made much larger than the markup that the author has to send

I'm not (yet) sure about (b) here.

(i.e. we care more about cases where a 1 MB HTML file could make us OOM, than about cases where a 4 GB file could make us OOM. In the latter case, it's often worth just using infallible allocators because of the error-checking complexity that it saves us. Do you know whether this is closer to the former or the latter case?)

Comment 4

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Seems like we can't do this unless the relevant code that modifies these
> arrays actually checks for whether allocation fails. (and that we behave
> sanely after that).
> 
> Is that the case?

Yes:

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

> (Side note: a while back, we made an intentional choice to use infallible
> allocators in the CSS parser, in bug 659963. That doesn't necessarily mean
> we have to be infallible here as well, though.)

We just need to be consistent here.  The above code clearly assumes the fallibility of these allocations.

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Also: at some point in the past, I remember being made aware of a
> rule-of-thumb that we should use fallible allocation when *both* of the
> following hold:
>   (a) data's size is author-controllable
>   (b) the data's size can be made much larger than the markup that the
> author has to send
> 
> I'm not (yet) sure about (b) here.

(b) will apply to nything that is controllable from content script (including CSS through the CSSOM and the DOM), right?

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8372849 [details] [diff] [review]
Use fallible allocation to store the style information for CSS transitions and animations

Oh, true. I was just thinking about static stylesheets as being the attack vector here, but I suppose you could have a JS loop that dynamically generates zillions of transitions/animations using the CSSOM.

And yup, it looks like fallible allocation is expected & failures are handled by the users of these arrays. Seems reasonable to explicitly make them fallible, given that.

Thanks!

Comment 6

[(no longer active)]

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/26d0e83eb70a

Comment 7

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #0)
> 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.

It's not clear to me which way the "mistake" was; there are tons of places where we have switched to infallible allocation but haven't gone through and removed the handling of allocation failure.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #0)
> > 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.
> 
> It's not clear to me which way the "mistake" was; there are tons of places
> where we have switched to infallible allocation but haven't gone through and
> removed the handling of allocation failure.

Well, I was referring to the fact that we were checking the result of SetLength() which would always be true.  If you think we should do infallible allocation here I would be happy to write that patch.

Comment 9

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

I indeed think these should be infallible, since the allocation sizes are a function of the size of the input (the number of array elements is a function of the number of comma-separated parts in the value of a CSS property, e.g., "transition-property: margin-left, margin-right, margin-top, margin-bottom, opacity"), which seems analogous to allocation of things like large numbers of nodes, with the only difference being that the allocation happens in a single chunk.

Comment 10

[(no longer active)]

(In reply to comment #9)
> I indeed think these should be infallible, since the allocation sizes are a
> function of the size of the input (the number of array elements is a function
> of the number of comma-separated parts in the value of a CSS property, e.g.,
> "transition-property: margin-left, margin-right, margin-top, margin-bottom,
> opacity"), which seems analogous to allocation of things like large numbers of
> nodes, with the only difference being that the allocation happens in a single
> chunk.

OK, I'll do that then.

Comment 11

[(no longer active)]

Attachment: Use infallible allocation to store the style information for CSS transitions and animations; r=dbaron

Comment 12

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

Comment on attachment 8372883 [details] [diff] [review]
Use infallible allocation to store the style information for CSS transitions and animations; r=dbaron

Thanks.  r=dbaron

Comment 13

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7e01ec0ab2

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26d0e83eb70a
https://hg.mozilla.org/mozilla-central/rev/2e7e01ec0ab2