1382964 - stylo: heap write hazards under CalcStyleDifference

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Cameron McCormack (:heycam)]

We've been whitelisting CalcStyleDifferenceInternal for heap write hazards.  In bug 1380133 CalcStyleDifferenceInternal was removed (and its work moved up into CalcStyleDifference) revealing some hazards.  For now, we're bumping the allowed hazards to 4 (in bug 1382956) but we should fix them.

The first I've encountered is URLValueData::MightHaveRef, which does some lazily initialization of mMightHaveRef.  That's not safe OMT.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1382964 - Part 1: Don't cache URLValueData::mMightHaveRef when in a traversal.

Review commit: https://reviewboard.mozilla.org/r/160138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160138/

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1382964 - Part 2: Assert we're on the main thread in nsCSSValueTokenStream::operator==.

This helps the heap write analysis believe that the nsCSSValue comparison
that nsStyleUIReset::CalcDifference does (for mSpecifiedWindowTransform,
which won't have nsCSSValueTokenStream values in it) is safe.

Review commit: https://reviewboard.mozilla.org/r/160140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160140/

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1382964 - Part 3: Add assertion to help heap write analysis.

Review commit: https://reviewboard.mozilla.org/r/160142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160142/

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1382964 - Part 4: Remove now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal.

Review commit: https://reviewboard.mozilla.org/r/160144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160144/

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1382964 - Part 5: Restore allowed heap write hazards to 3.

Review commit: https://reviewboard.mozilla.org/r/160146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160146/

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b09c3ef0ee516e37e9dc8386f58a95453f2f0086&group_state=expanded

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8889116 [details]
Bug 1382964 - Part 1: Don't cache URLValueData::mMightHaveRef when in a traversal.

https://reviewboard.mozilla.org/r/160138/#review165498

::: layout/style/nsCSSValue.cpp:2909
(Diff revision 1)
> -  return false;
> +  if (mMightHaveRef.isNothing()) {
> +    mMightHaveRef.emplace(result);
> +  }

`mMightHaveRef = Some(result);` might be enough.

::: layout/style/nsCSSValue.cpp:2920
(Diff revision 1)
> -    // ::MightHaveRef is O(N), use it only use it only when MightHaveRef is
> -    // called.
> -    mMightHaveRef.emplace(::MightHaveRef(mString));
> +    bool result = ::MightHaveRef(mString);
> +    if (!ServoStyleSet::IsInServoTraversal()) {
> +      // Can only cache the result if we're not on a style worker thread.
> +      mMightHaveRef.emplace(result);
> +    }

I guess this is fine... but could be improved. We can probably make the field atomic so that it is safe to operate during traversal.

That may require some new class in mfbt I guess... so can be done separately.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8889117 [details]
Bug 1382964 - Part 2: Assert we're on the main thread in nsCSSValueTokenStream::operator==.

https://reviewboard.mozilla.org/r/160140/#review165500

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8889118 [details]
Bug 1382964 - Part 3: Add assertion to help heap write analysis.

https://reviewboard.mozilla.org/r/160142/#review165502

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8889119 [details]
Bug 1382964 - Part 4: Remove now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal.

https://reviewboard.mozilla.org/r/160144/#review165504

::: commit-message-adb37:1
(Diff revision 1)
> +Bug 1382964 - Part 4: Now now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal. r?xidorn

Now **remove** unused heap write hazard?

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8889120 [details]
Bug 1382964 - Part 5: Restore allowed heap write hazards to 3.

https://reviewboard.mozilla.org/r/160146/#review165506

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8889116 [details]
Bug 1382964 - Part 1: Don't cache URLValueData::mMightHaveRef when in a traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160138/diff/1-2/

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8889117 [details]
Bug 1382964 - Part 2: Assert we're on the main thread in nsCSSValueTokenStream::operator==.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160140/diff/1-2/

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8889118 [details]
Bug 1382964 - Part 3: Add assertion to help heap write analysis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160142/diff/1-2/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8889119 [details]
Bug 1382964 - Part 4: Remove now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160144/diff/1-2/

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8889120 [details]
Bug 1382964 - Part 5: Restore allowed heap write hazards to 3.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160146/diff/1-2/

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8889119 [details]
Bug 1382964 - Part 4: Remove now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160144/diff/2-3/

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8889120 [details]
Bug 1382964 - Part 5: Restore allowed heap write hazards to 3.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160146/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fed5a7719c1
Part 1: Don't cache URLValueData::mMightHaveRef when in a traversal. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/65fb1f76ed5b
Part 2: Assert we're on the main thread in nsCSSValueTokenStream::operator==. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b9d6e44a276f
Part 3: Add assertion to help heap write analysis. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/0f27092c9bd4
Part 4: Remove now unused heap write hazard whitelist entry for CalcStyleDifferenceInternal. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b1e6304c5427
Part 5: Restore allowed heap write hazards to 3. r=xidorn

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fed5a7719c1
https://hg.mozilla.org/mozilla-central/rev/65fb1f76ed5b
https://hg.mozilla.org/mozilla-central/rev/b9d6e44a276f
https://hg.mozilla.org/mozilla-central/rev/0f27092c9bd4
https://hg.mozilla.org/mozilla-central/rev/b1e6304c5427