659963 - Consistently use infallible memory allocation in CSS parser

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

Some, but by no means all, memory allocation points in the CSS parser try to recover from allocation failure by setting a "low level error" flag to NS_ERROR_OUT_OF_MEMORY and then returning PR_FALSE to higher layers.  Higher layers invariably treat a PR_FALSE return from a parser subroutine as indicating a *syntax* error, so the behavior on allocation failure is likely to be to attempt syntax error recovery, inappropriately, and then barge onwards.  The very highest parser functions do check the "low level error" and report to their callers, but nearly every *caller* of those functions ignores error returns.  So, in summary, it's a big mess.  I propose to rip the whole mess out and just rely on infallible operator new, infallible nsTArray resize, etc. throughout nsCSSParser.

This was originally part of bug 543151 and probably has a textual dependency on the current patch series in that bug.  I recall folks expressing skepticism about it (but not the details - sorry, it was some time ago) so I'm pulling it out to its own bug.

This is not an exhaustive audit of memory allocation points in layout/style, it only covers allocations that could cause a call to nsCSSScanner::SetLowLevelError.

Comment 1

[Benjamin Smedberg]

In general, when web content can control the allocation sizes, we do not want to use the infallible allocator. This seems like one of the obvious cases (apart from the code perhaps being incorrect currently) where we *do* want a fallible allocator.

Comment 2

[Zack Weinberg (:zwol)]

Consistent use of fallible allocation is not feasible with the code as it stands: every call site in the parser would have to be changed to detect and propagate memory allocation failures.  Inconsistent behavior upon allocation failure is, IMO, worse than consistent use of infallible allocation, even for allocations directly caused by content.

Also, the amount of memory allocated for the parse of a style sheet is at most linear in the size of the sheet itself; this is not like images or script execution, where a small file can cause arbitrarily large allocations.  (It is probably comparable to script *parsing* - I don't know how that works.)

Comment 3

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

Comment on attachment 535357 [details] [diff] [review]
patch

In the else-if chain in ParseColorString, please change the else-ifs
to just ifs, and make the first if return NS_ERROR_FAILURE inside the
outer if but outside the inner if.

Could you make nsMediaQuery::mExpressions be explicitly
InfallibleTArray<> rather than nsTArray<> (even though they're now
the same).

Same for nsMediaList::mArray.

Please change the return type of CSSParserImpl::PushGroup to return
void, and likewise change mGroupStack to InfallibleTArray.

In ParsePseudoSelector, do_GetAtom is currently fallible.  So please
add NS_RUNTIMEABORT there as well.

Likewise, make nsCSSValueGradient::mStops InfallibleTArray.

Make CSSParserImpl::ParseFunctionInternals take InfallibleTArray,
and ParseFunction use it.

r=dbaron with that

Comment 4

[Zack Weinberg (:zwol)]

Attachment: v2

(In reply to comment #3)
> In the else-if chain in ParseColorString, please change the else-ifs
> to just ifs, and make the first if return NS_ERROR_FAILURE inside the
> outer if but outside the inner if.

Done.

> Could you make nsMediaQuery::mExpressions be explicitly
> InfallibleTArray<> rather than nsTArray<> (even though they're now
> the same).
[etc]

After rereading the code I changed all uses of nsTArray<> in the parser to InfallibleTArray<>.  There were several places that were already assuming infallible resize semantics.

> Please change the return type of CSSParserImpl::PushGroup to return
> void, and likewise change mGroupStack to InfallibleTArray.

Done.  Sorry about that, I meant to do it but forgot.

> In ParsePseudoSelector, do_GetAtom is currently fallible.  So please
> add NS_RUNTIMEABORT there as well.

Done (also several other places where do_GetAtom was being called without any check).

Given slightly more changes than you asked for, do you want to re-review?

Comment 5

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

Comment on attachment 535481 [details] [diff] [review]
v2

r=dbaron

Comment 6

[Zack Weinberg (:zwol)]

Attachment: v2 updated and checked in

Dusted this off (some skew, but no significant changes) and pushed to -inbound with the first two pieces of bug 543151.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67673422f7d2

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/67673422f7d2