585867 - Remove OOM checks from property getters in nsComputedDOMStyle and make them not return nsresult

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Craig Topper]

The property getters in nsComputedDOMStyle spend a lot of code handling OOM errors. Since allocation is now infallible all these checks can be removed simplifying the code and allowing the getters to lose their outparam.

Comment 1

[Craig Topper]

Attachment: Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList

Comment 2

[Craig Topper]

Attachment: Part 2: Make SetToRGBAColor return void

Comment 3

[Craig Topper]

Attachment: Part 3: Make AppendCSSValue infallible

Unfortunately, nsCOMArray and nsTArray are not infallible during resize operations. To work around this I have added my own call to mozalloc_handle_oom which will abort making us infallible. I modeled this after the code for moz_xmalloc.

Comment 4

[Craig Topper]

Attachment: Part 4: Make SetValueToStyleImage return void

Comment 5

[Craig Topper]

Attachment: Part 5: Remove outparam and AddRef from all property getters

This patch removes the outparam from all of the property getters and moves to a single AddRef after the getter returns.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 466927 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>--- a/layout/style/nsComputedDOMStyle.cpp
>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -661,23 +661,22 @@ nsComputedDOMStyle::DoGetStackSizing(nsI
> 
>   val->SetIdent(GetStyleXUL()->mStretchStack ? eCSSKeyword_stretch_to_fit :
>                 eCSSKeyword_ignore);
> 
>   NS_ADDREF(*aValue = val);
>   return NS_OK;
> }
> 
>-nsresult
>+void
> nsComputedDOMStyle::SetToRGBAColor(nsROCSSPrimitiveValue* aValue,
>                                    nscolor aColor)
> {
>   if (NS_GET_A(aColor) == 0) {
>     aValue->SetIdent(eCSSKeyword_transparent);
>-    return NS_OK;
>   }

You still need the return, right?

Comment 7

[Craig Topper]

> You still need the return, right?

Oops! Thanks!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Craig, are those patches pretty much current (in that I should go ahead and review them), or have you had to merge since you posted them in nontrivial ways?

Comment 9

[Craig Topper]

I can post newer patches. Can't remember if any of the merges were non trivial.

Comment 10

[Craig Topper]

Attachment: Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList

Comment 11

[Craig Topper]

Attachment: Part 2: Make SetToRGBAColor return void

Comment 12

[Craig Topper]

Attachment: Part 3: Make AppendCSSValue infallible

This patch converts the nsCOMArray<nsIDOMCSSValue> in nsDOMCSSValueList to an infallible nsTArray<nsCOMPtr<nsIDOMCSSValue>>.

Comment 13

[Craig Topper]

Attachment: Part 4: Make SetValueToStyleImage return void

Comment 14

[Craig Topper]

Attachment: Part 5: Remove outparam and AddRef from all property getters

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500641 [details] [diff] [review]
Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500642 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500643 [details] [diff] [review]
Part 3: Make AppendCSSValue infallible

Why do you need the mozilla/mozalloc_oom.h include in nsDOMCSSValueList.cpp?  r=me with that explained or removed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, that looks like a leftover from the previous patch version.  Just remove it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500644 [details] [diff] [review]
Part 4: Make SetValueToStyleImage return void

r=me

Comment 20

[Craig Topper]

(In reply to comment #17)
> Comment on attachment 500643 [details] [diff] [review]
> Part 3: Make AppendCSSValue infallible
> 
> Why do you need the mozilla/mozalloc_oom.h include in nsDOMCSSValueList.cpp? 
> r=me with that explained or removed.

Oops that's leftover from before I converted to nsTArray. I was previously aborting myself since nsTArray wasn't infallible.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500646 [details] [diff] [review]
Part 5: Remove outparam and AddRef from all property getters

The changes to nsComputedDOMStyle::GetEllipseRadii change behavior (by changing the call to SetValueToCoord).  Merge issue?

Other than that, document in the struct in the header the ownership model for these getters (returned pointer is to a refcounted object that's just been created, with a refcount still at 0).

r=me with those two things fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Note that this will end up conflicting with bug 256274 (which adds some stuff to nsComputedDOMStyle).  I think we should land this first, since the merge in the other direction should be simpler.  None of this is an issue until the tree reopens, of course....

Comment 23

[Craig Topper]

Attachment: Part 3: Make AppendCSSValue infallible

Patch to address review comments

Comment 24

[Craig Topper]

Attachment: Part 5: Remove outparam and AddRef from all property getters

Addresses review comments

Comment 25

[Boris Zbarsky [:bzbarsky]]

Craig, thanks for updating those!

Pushed to birch:
           http://hg.mozilla.org/projects/birch/rev/2366a35dd7ce
           http://hg.mozilla.org/projects/birch/rev/a4b02f48bd0e
           http://hg.mozilla.org/projects/birch/rev/47487a9c4b11
           http://hg.mozilla.org/projects/birch/rev/f1762e209d6c
           http://hg.mozilla.org/projects/birch/rev/39ec013827f5

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/2366a35dd7ce
https://hg.mozilla.org/mozilla-central/rev/a4b02f48bd0e
https://hg.mozilla.org/mozilla-central/rev/47487a9c4b11
https://hg.mozilla.org/mozilla-central/rev/f1762e209d6c
https://hg.mozilla.org/mozilla-central/rev/39ec013827f5