add nsCSSRuleProcessor constructor that takes a sheet array rvalue reference

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8740898 [details] [diff] [review]
patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84a1faed3ef
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8740898 - Flags: review?(bbirtles)
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?
(Assignee)

Comment 3

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

Comment 5

3 years ago
(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 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.
Attachment #8740898 - Flags: review?(bbirtles) → review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cbb08180a3ec
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.