1234707 - Avoid passing objects with a refcount of 0 to nsDOMCSSValueList::AppendCSSValue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

nsComputedDOMStyle has a lot of calls to nsDOMCSSValueList::AppendCSSValue().

Right now, in every one of those calls, it passes in a refcounted object with a refcount of 0. The refcounted object doesn't get a nonzero refcount until AppendCSSValue places it in its internal array-of-RefPtr.  Until that point, we're kind of playing with fire.

We should just follow our best practices and stick these objects in a RefPtr as soon as possible, and make nsDOMCSSValueList::AppendCSSValue adopt ownership of that object.

See also bug 1234676 (which is related, and whose patches I'll layer on top of this bug's patch).

(Background/motivation: it's dangerous to be passing around refcounted objects that have a refcount of 0. If anyone (e.g. a helper-function that we invoke) happens to wrap the object in a temporary RefPtr, then the object will be deleted when that temporary variable goes out of scope, and we'll end up with a use-after-free on any subsequent usages of the object.  We can avoid this by putting the object in a RefPtr<> as soon as possible after it's allocated, and making sure that all ownership hand-offs from that point on use already_AddRefed or RefPtr types.)

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

One way of doing this would be to just wrap everything we pass to AppendCSSValue() in a RefPtr, e.g.
  {
    RefPtr<nsROCSSPrimitiveValue> foo = new nsROCSSPrimitiveValue;
    [...code that sets up |foo|]
    valueList.AppendCSSValue(foo.get()); // foo's refcount goes up to 2
  } // foo goes out of scope and decrements its refcount back to 1

HOWEVER, this results in some refcount churn -- going up to 2 and then back down to 1. (which is extra churn on top of what we're doing right now)

We can avoid this churn by making AppendCSSValue() take an already_AddRefed, and calling "forget" on the argument at the callsites. This makes AppendCSSValue() take ownership of whatever's been passed to it.  This is worth doing, IMO, to avoid the refcount churn noted above.  So this version looks like:
  {
    RefPtr<nsROCSSPrimitiveValue> foo = new nsROCSSPrimitiveValue;
    [...code that sets up |foo|]
    valueList.AppendCSSValue(foo.forget()); // foo is nulled out, and its refcount stays at 1
  }


This means we have to swap lines around at the callsites a bit so that we always build up a value *before* calling AppendCSSValue.  Fortunately, these swaps are pretty trivial.  As a bonus, this already_AddRefed<> usage forces us to use a RefPtr at *all* of the callsites, too. (It prevents us from just allocating a value and sticking it in a raw pointer and passing that in, as we do currently.)

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: wip

Here's an initial patch (layering on top of current mozilla-inbound), which at least builds successfully and passes ./mach mochitest layout/style/test/test_value_computation.html, which probably means it's correct.

I'm not requesting review quite yet, because I want to do a manual sanity-check pass myself first.

Comment 3

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

Another possibility is to use RefPtr<T> rvalue references and Move(), although I fear that might not produce compiler errors if people do the wrong (extra-refounting) thing.  If it does, though, it might be preferable.

Comment 4

[Cameron McCormack (:heycam)]

I was just about to suggest rvalue references too, but they're probably not as flexible as already_AddRefed.  For example I'm not sure if the following would compile:

  RefPtr<nsROCSSPrimitiveValue> v = new nsROCSSPrimitiveValue;
  v->SetString(...);
  valueList->AppendCSSValue(Move(v));

if AppendCSSValue is declared to take a RefPtr<CSSValue>&&.

Making AppendCSSValue a function template could solve that, of course.  As could making the local variable be a RefPtr<CSSValue>, but then you need casting or something to be able to call nsROCSSPrimitiveValue methods.

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Yeah, I'd lean towards sticking with already_AddRefed -- for now at least -- for the reasons heycam brought up.  (I'm not sure it's worthwhile to make AppendCSSValue a templated function, just so we can use rvalue references with the various flavors of RefPtr<CSSValue>, when already_AddRefed works on its own.)

Also: after taking this bug's current already_AddRefed patch, we could trivially switch to Move in a future bug (if we decide it's better) by simply doing search & replace for s/AppendCSSValue(XYZ.forget())/AppendCSSValue(Move(XYZ))/.  The patch here is largely about (a) storing these resources in RefPtr<> in the first place, and (b) not calling AppendCSSValue until we're done setting up the value.  And those pieces will remain the same regardless of what type of smart-pointer we use in AppendCSSValue.

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

For ease of review, I'll be splitting the patch into 4 conceptually-distinct chunks. We can't compile unless we've got all of them, so I'll fold them together before landing, though.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: part 1: change the API

This part just changes the nsDOMCSSValueList::AppendCSSValue API, without updating any of the callers.

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: part 2: trivial, in-place changes

This part is the most trivial fixes -- fixes that can be made in-place, without reordering any code.  So, this patch is just editing some lines to add RefPtr<> and .forget() invocations.

(The one slightly interesting piece of this patch is the change to CreatePrimitiveValueForStyleFilter() -- that function is *only* used to provide something that we pass to AppendCSSValue. So, I've updated it to return type already_AddRefed.  This is an in-place change -- no code-reordering or anything fancy needed -- so I've grouped it as part of this patch.)

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: part 3: Changes that require moving the AppendCSSValue() call down a bit

This patch is entirely to fix code of the following form:
  RefPtr<nsROCSSPrimitiveValue> foo = new nsROCSSPrimitiveValue;
  valueList.AppendCSSValue(foo);
  [...Code that sets up foo...]

Now that AppendCSSValue will be taking ownership of |foo|, it needs to happen *after* the "Code that sets up foo".  This patch does that, for every instance of this pattern.

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: part 4: changes which require some special handling (and will be better after bug 1234676)

This patch is for a last few cases which currently require some special handling (and temporary local variables) until bug 1234676 is fixed.

Specifically:
 - We have to wrap some calls to GetGridTrackSize in a local RefPtr in order to produce an already_AddRefed for an AppendCSSValue call. Really, GetGridTrackSize should return already_AddRefed, but it can't yet because it's used directly by some DoGetXYZ accessors (DoGetGridAutoColumns/Rows), and those functions rely on their returned thing having a refcount of 0.
 - In CreateTextAlignValue, we have to return something with a refcount of 0 (again, because we're returning it to a DoGetXYZ function) -- and local variable |val| may be that thing we return, or it may be something that we pass to AppendCSSValue. For now, we only wrap it in a RefPtr if we pass it to AppendCSSValue.
 - In DoGetTextDecoration, we directly invoke some other DoGetXYZ functions and pass their return value to AppendCSSValue. We can't do that anymore, now that AppendCSSValue takes an already_AddRefed -- we need to store the result in a local RefPtr and then call forget().

All of these special cases & local variables can be eliminated once we fix bug 1234676 and make the DoGetXYZ function return an already_AddRefed value.

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: rollup patch w/ commit message

Here's the rollup patch, with all 4 parts folded together. Commit message:
Bug 1234707: Make nsDOMCSSValueList::AppendCSSValue() take an already_AddRefed arg (instead of a raw pointer, usually with refcount of 0). r?

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(oops, my description-list in comment 10 was missing an overview of the final part, for GetSVGPaintFor. That one's pretty similar to the others, anyway.)

Comment 13

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Thanks for the quick review turnaround here!

I gave my WIP patch a Try run yesterday, and I'm pretty sure the test failures are all intermittent/unrelated:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e493084f7ca

Not much changed between the WIP and the final version, so I don't think it needs another Try run. Landing shortly!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64671f01cb7b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64671f01cb7b