Closed Bug 1382964 Opened 2 years ago Closed 2 years ago

stylo: heap write hazards under CalcStyleDifference

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files)

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 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.
Attachment #8889116 - Flags: review?(xidorn+moz) → review+
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
Attachment #8889117 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8889118 [details]
Bug 1382964 - Part 3: Add assertion to help heap write analysis.

https://reviewboard.mozilla.org/r/160142/#review165502
Attachment #8889118 - Flags: review?(xidorn+moz) → review+
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?
Attachment #8889119 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8889120 [details]
Bug 1382964 - Part 5: Restore allowed heap write hazards to 3.

https://reviewboard.mozilla.org/r/160146/#review165506
Attachment #8889120 - Flags: review?(xidorn+moz) → review+
Pushed by cmccormack@mozilla.com:
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
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.