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

RESOLVED FIXED in mozilla30

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

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 8372849 [details] [diff] [review]
Use fallible allocation to store the style information for CSS transitions and animations
(Assignee)

Updated

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

Updated

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

Comment 4

4 years ago
(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 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!
Attachment #8372849 - Flags: review?(dholbert) → review+
(Assignee)

Comment 6

4 years ago
Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/26d0e83eb70a
(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.
(Assignee)

Comment 8

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

Comment 10

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

Comment 11

4 years ago
Created attachment 8372883 [details] [diff] [review]
Use infallible allocation to store the style information for CSS transitions and animations; r=dbaron
(Assignee)

Updated

4 years ago
Attachment #8372849 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8372883 - Flags: review?(dbaron)
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
Attachment #8372883 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7e01ec0ab2
https://hg.mozilla.org/mozilla-central/rev/26d0e83eb70a
https://hg.mozilla.org/mozilla-central/rev/2e7e01ec0ab2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.