1348606 - stylo: Gecko_CopyStyleContentsFrom modifies non-threadsafe refcounts

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Bobby Holley (:bholley)]

See bug 1294915 comment 51. I think we probably have no choice but to give the arrays in nsStyleContentsData threadsafe refcounts.

Comment 1

[Bobby Holley (:bholley)]

I have partial patches here. It's quite annoying to set things up so that we use threadsafe refcounts for counter/counters but not stuff like calc, but I think I'm getting close. Will finish up later.

Comment 2

[Bobby Holley (:bholley)]

Though I guess that begs the question: Do we have a plan for implementing inheritance for the other array-valued things in nsCSSValue (See things starting at [1] and beyond)? Or will we eventually need threadsafe refcounts for all those things to implement copy_*_from?

[1] http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/layout/style/nsCSSValue.h#446

Comment 3

[Manish Goregaokar [:manishearth]]

> Do we have a plan for implementing inheritance for the other array-valued things in nsCSSValue (See things starting at [1] and beyond)? Or will we eventually need threadsafe refcounts for all those things to implement copy_*_from?

Most of those are only used in specified values so we should be fine. I don't think we ever clone gecko specified values off main thread except for the ones which are also used in the style struct (counters, transform, etc).

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Use the usual macro for the refcounts in nsCSSValue::Array. v1

MozReview-Commit-ID: K3FlZIDymBD

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Get rid of CSSVALUE_LIST_FOR_EXTRA_VALUES. v1

This makes it more annoying to put the class definition in a macro,
which we do in the next patch.

MozReview-Commit-ID: 443V7z4sMfi

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Inline nsCSSValue::Array::SizeOfIncludingThis. v1

This will make it easier to macro-ize this stuff.

MozReview-Commit-ID: Hcmg6yaTTdt

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Turn nsCSSValue::Array into a macro. v1

MozReview-Commit-ID: LZDUi6kwWH5

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Use a threadsafe array for counters. v1

MozReview-Commit-ID: KgTgcD5mGqr

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8850296 [details] [diff] [review]
Part 1 - Use the usual macro for the refcounts in nsCSSValue::Array. v1

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

So initially, this uses an 16bit int to save space, and when we start using the macro (bug 551298), it is not replaced for the same reason. But then because of a CVE (bug 574059), refcount of nsCSSValue::Array is switched to use size_t, so it is safe to use the macro now :)

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

I'm not really a fan of changing a long struct into a macro...

Could we do something similar to our nsString vs. nsCString trick which uses an extra file as the template, then define macros to replace things inside?

I think that would make it clearer and part 2 would be no longer necessary.

Actually how bad would it be if we just change nsCSSValue::Array to use thread-safe counter?

Comment 12

[Bobby Holley (:bholley)]

Per IRC:
(1) We use Array for a lot of stuff [1]. I'd like to reduce perf regression risk.
(2) A separate file might have some advantages, but I already did it this way, and have a lot on my plate, so I would much rather land this and move on. This code may all change when we rip out the gecko style system anyway.

[1] http://searchfox.org/mozilla-central/search?q=symbol:_ZNK10nsCSSValue13GetArrayValueEv&redirect=false

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8850297 [details] [diff] [review]
Part 2 - Get rid of CSSVALUE_LIST_FOR_EXTRA_VALUES. v1

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

Actually you can simply move this macro out from the class definition...

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8850300 [details] [diff] [review]
Part 5 - Use a threadsafe array for counters. v1

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

You probably need to update the binding files with your Servo side change.

::: layout/style/nsCSSValue.h
@@ +443,5 @@
>    eCSSUnit_Font_Format  = 16,     // (char16_t*) a font format name
>    eCSSUnit_Element      = 17,     // (char16_t*) an element id
>  
> +  eCSSUnit_Counter      = 20,     // (nsCSSValue::Array*) a counter(string,[string]) value
> +  eCSSUnit_Counters     = 21,     // (nsCSSValue::Array*) a counters(string,string[,string]) value

nsCSSValue::ThreadSafeArray*

Comment 15

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> Comment on attachment 8850297 [details] [diff] [review]
> Part 2 - Get rid of CSSVALUE_LIST_FOR_EXTRA_VALUES. v1
> 
> Review of attachment 8850297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually you can simply move this macro out from the class definition...

I considered doing that, but eventually decided that the macro was introducing more complexity than it was reducing, given that it's only used in two adjacent callsites.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f6b38dab400
Use the usual macro for the refcounts in nsCSSValue::Array. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/51153d4d5e93
Get rid of CSSVALUE_LIST_FOR_EXTRA_VALUES. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/544d1b37f031
Inline nsCSSValue::Array::SizeOfIncludingThis. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/49f0cca10106
Turn nsCSSValue::Array into a macro. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/cab92aa7f83e
Use a threadsafe array for counters. r=xidorn

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f6b38dab400
https://hg.mozilla.org/mozilla-central/rev/51153d4d5e93
https://hg.mozilla.org/mozilla-central/rev/544d1b37f031
https://hg.mozilla.org/mozilla-central/rev/49f0cca10106
https://hg.mozilla.org/mozilla-central/rev/cab92aa7f83e