1247182 - add nsCSSRuleProcessor constructor that takes a sheet array rvalue reference

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

There are a few call sites where we really need to pass ownership of the sheet array to the newly created nsCSSRuleProcessor, so we should have a constructor that takes an nsTArray<RefPtr<CSSStyleSheet>>&&.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84a1faed3ef

Comment 2

[Brian Birtles (:birtles)]

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

Review of attachment 8740898 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSRuleProcessor.cpp
@@ +983,5 @@
>                                         Element* aScopeElement,
>                                         nsCSSRuleProcessor*
>                                           aPreviousCSSRuleProcessor,
>                                         bool aIsShared)
> +  : nsCSSRuleProcessor(Move(sheet_array_type(aSheets)), aSheetType,

I'm probably overlooking something but I don't understand why this is ok.

As I understand it, Move() basically does a static cast to sheet_array_type&&. The call site of this constructor, however, is passing in a const-ref and might be assuming that aSheets will be untouched as a result of calling this constructor. However, if we pass aSheets to the nsCSSRuleProcessor overload that takes an rvalue reference, then initialize mSheets with the move constructor for nsTArray, that will call SwapElements and leave aSheets empty, right? So if the call site assumes it can keep using aSheets it will get a surprise?

If that's right, maybe we need to make one constructor do copying and one do moving, either by duplicating the logic (i.e. not using delegated constructors) or by factoring out a third base constructor that doesn't touch mSheets? Alternatively we could have a templated constructor that takes advantages of universal references to provide the different behaviors.

Alternatively, if we don't need the copy behavior at all we should just make all call sites explicitly pass an rvalue reference so they're declaring they're not going to use aSheets anymore.

Is that right?

Comment 3

[Cameron McCormack (:heycam)]

The sheet_array_type() in there should be making a copy of aSheets, which is what gets moved into mSheets.  Certainly if aSheets were modified, then all kinds of things would break (since this constructor gets called with some of nsStyleSet's sheet arrays).

Comment 4

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> The sheet_array_type() in there should be making a copy of aSheets, which is
> what gets moved into mSheets.  Certainly if aSheets were modified, then all
> kinds of things would break (since this constructor gets called with some of
> nsStyleSet's sheet arrays).

Ah, thanks yes! I didn't recognize that as a constructor! In that case, we shouldn't need Move(), right? It should already be an rvalue?

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Ah, thanks yes! I didn't recognize that as a constructor! In that case, we
> shouldn't need Move(), right? It should already be an rvalue?

Ah yes, I'll try that.

Comment 6

[Brian Birtles (:birtles)]

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

Review of attachment 8740898 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSRuleProcessor.cpp
@@ +983,5 @@
>                                         Element* aScopeElement,
>                                         nsCSSRuleProcessor*
>                                           aPreviousCSSRuleProcessor,
>                                         bool aIsShared)
> +  : nsCSSRuleProcessor(Move(sheet_array_type(aSheets)), aSheetType,

As discussed, we should drop the Move() if that works.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb08180a3ec

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbb08180a3ec