Closed Bug 1356103 Opened 7 years ago Closed 7 years ago

stylo: Font metrics aren't thread safe

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: manishearth, Assigned: heycam)

References

Details

Attachments

(12 files)

Bug 1356103 - Part 1: Add WeakPtrTraits to allow SupportsWeakPtr classes to opt in to more permissive thread ownership assertions.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 2: Add functions to assert the Servo font metrics mutex is locked.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 3: Make it easy to access the ServoStyleSet currently in traversal.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 4: Add a mechanism for C++ functions to perform post-Servo traversal tasks.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 5: Allow access to WeakPtr<UnscaledFont> when the Servo font metrics mutex is locked.
59 bytes, text/x-review-board-request
lsalzman
: review+
Details
Bug 1356103 - Part 6: Make gfxUserFontSet refcounting thread-safe.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 8: Use PostTraversalTasks to deal with FontFaceSet's Promise and DOM event dispatch during Servo traversal.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 9: Use a PostTraversalTask to deal with downloadable fonts in gfxUserFontSet.
59 bytes, text/x-review-board-request
bholley
: review+
jfkthame
: review+
Details
Bug 1356103 - Part 10: Re-enable font metrics querying for ch and ex units in Servo traversal.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 11: Adjust text expectations.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356103 - Part 12: Allow OMT heap writes under Gecko_GetFontMetrics.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
See bug 1355811, bug 1355814, and maybe bug 1355813

Font metrics touch dom::FontFaceSet, which does things like frobbing the dom and network.

We shouldn't do that. The plan is to instead queue up a sequential task for these in the cases where we wish to trigger a network load.
Blocks: 1356105
(I'm going to look into whether we can make the UserFontSet work in a mode where it doesn't do new font loads, and instead reports this back to Servo, which can then post a SequentialTask to perform that work, and potentially do another restyle with the elements that relied on the font metrics that otherwise would have been synchronously returned, as is done for data: URIs, platform fonts, ...)
Flags: needinfo?(cam)
Priority: -- → P1
Maybe we can avoid digging to find all of the OMT-unsafe work that nsFontMetrics might perform and retrofitting gfxPlatform, gfxFontGroup, gfxUserFontSet, etc. with a mode that does the OMT-safe work but returns with an error if it needed to do OMT-unsafe work.  Instead, we could just rely on the nsFontCache.  If we find an entry in the nsFontCache, great, just return it.  If not, then we return null back to Servo, which can then post a SequentialTask with (a) all the details required to do the nsRuleNode::GetMetricsFor call again, and (b) the element we were styling, so we can post a restyle for it.

Does that sound reasonable?  The @media use of font metrics might need to be handled differently.
Flags: needinfo?(cam) → needinfo?(manishearth)
One issue is that the nsFontCache eagerly removes an entry from its mFontMetrics array when an nsFontMetrics object is deleted.  Because the SequentialTask would create an nsFontCache entry but otherwise have nowhere to store it, we might need change how the cache operates so that it sticks around long enough for the subsequent restyle to pick it out.

Or, we should do some caching ourselves on the Servo side.  I'm not actually sure if something in Gecko will hold on to that same nsFontMetrics object so that if we need to restyle the element with the cx/ch length, that we don't have to create a new nsFontMetrics object again.  So perhaps having a cache of GeckoFontMetrics objects (either on the Gecko or Servo side), whose expiration we could manage differently from nsFontCache, would be better.
Also, this might obviate the need to call GetUserFontSet in ServoStyleSet::PreTraverseSync.
(In reply to Cameron McCormack (:heycam) from comment #2)
> Maybe we can avoid digging to find all of the OMT-unsafe work that
> nsFontMetrics might perform and retrofitting gfxPlatform, gfxFontGroup,
> gfxUserFontSet, etc. with a mode that does the OMT-safe work but returns
> with an error if it needed to do OMT-unsafe work.  Instead, we could just
> rely on the nsFontCache.  If we find an entry in the nsFontCache, great,
> just return it.  If not, then we return null back to Servo, which can then
> post a SequentialTask with (a) all the details required to do the
> nsRuleNode::GetMetricsFor call again, and (b) the element we were styling,
> so we can post a restyle for it.

How often would we need to post these restyles? I'm worried about perf cliffs when somebody adds a ch unit.
We'd need to post the restyle each time we do a traversal and we compute a length that uses a ch/ex unit for a particular font and style that we haven't encountered before.  And when a font finishes downloading (under nsPresContext::UserFontSetUpdated, although we could try to be clever and see which font metrics have already cached, and refresh those on the main thread before kicking off the restyle.

If font properties are being animated on the element that uses ex/ch values on some property, then each tick would require that additional traversal, since basically all of the font properties are part of the font metrics cache key...
I'm going to hack something comment 2-like up quickly and see how it performs.
I didn't get to something that runs, but I got far enough to think that it's not going to be great having the extra traversal even when there are no downloadable fonts involved.  And with animations potentially involved, we potentially would need an extra traversal after the first (normal) traversal and after the traversals that were queued up due to animations.  I'll go back to tracing through to see if we can just queue up main thread work for cases involving downloadable fonts.
The reftest failures in the try run are bug 1361013.
Comment on attachment 8863202 [details]
Bug 1356103 - Part 5: Allow access to WeakPtr<UnscaledFont> when the Servo font metrics mutex is locked.

https://reviewboard.mozilla.org/r/135002/#review138488

Redirecting this to Lee, who I think is much more familiar with UnscaledFont.
Attachment #8863202 - Flags: review?(jfkthame) → review?(lsalzman)
Comment on attachment 8863202 [details]
Bug 1356103 - Part 5: Allow access to WeakPtr<UnscaledFont> when the Servo font metrics mutex is locked.

https://reviewboard.mozilla.org/r/135002/#review138518
Attachment #8863202 - Flags: review?(lsalzman) → review+
Comment on attachment 8863206 [details]
Bug 1356103 - Part 9: Use a PostTraversalTask to deal with downloadable fonts in gfxUserFontSet.

https://reviewboard.mozilla.org/r/135010/#review138548

I worry slightly that making data-url font loads potentially async could have performance repercussions, as until now "embedding" a webfont in a data-url has been a way authors could avoid the possible pitfalls of async loading, but I guess it should be OK. I don't think the behavior has ever been tightly specified, it's just something we did as an optimization.

::: layout/style/PostTraversalTask.h:82
(Diff revision 2)
>      DispatchLoadingEventAndReplaceReadyPromise,
>  
>      // mTarget (FontFaceSet*)
>      DispatchFontFaceSetCheckLoadingFinishedAfterDelay,
> +
> +    // mTarget (gfxFontEntry*)

s/gfxFontEntry/gfxUserFontEntry/
Attachment #8863206 - Flags: review?(jfkthame) → review+
Comment on attachment 8863198 [details]
Bug 1356103 - Part 1: Add WeakPtrTraits to allow SupportsWeakPtr classes to opt in to more permissive thread ownership assertions.

https://reviewboard.mozilla.org/r/134994/#review138934
Attachment #8863198 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863199 [details]
Bug 1356103 - Part 2: Add functions to assert the Servo font metrics mutex is locked.

https://reviewboard.mozilla.org/r/134996/#review138936

::: layout/style/ServoBindings.cpp:1846
(Diff revision 2)
> +    MOZ_DIAGNOSTIC_ASSERT(sServoFontMetricsLock);
> +
> +    // Ideally this would call something that does MOZ_DIAGNOSTIC_ASSERT, since
> +    // that's what WeakPtr does for its own thread ownership checks, but Mutex
> +    // only exposes a MOZ_ASSERT-level assertion function.
> +    sServoFontMetricsLock->AssertCurrentThreadOwns();

TBH I don't think it's worth making the null check a diagnostic assert, especially of the AssertCurrentThreadOwns() isn't one. I'm fine with just making all of this stuff regular MOZ_ASSERts and dropping the comments. I think the weak pointer diagnostic asserts are mostly to catch usage in poorly-tested code, and the style ssytem is very well tested.

::: layout/style/ServoUtils.h:17
(Diff revision 2)
> +void AssertIsMainThreadOrServoFontMetricsLocked();
> +void DiagnosticAssertIsMainThreadOrServoFontMetricsLocked();

I think this second one is erroneous? I don't see a function for it in this patch.
Attachment #8863199 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863200 [details]
Bug 1356103 - Part 3: Make it easy to access the ServoStyleSet currently in traversal.

https://reviewboard.mozilla.org/r/134998/#review138938

::: layout/style/ServoStyleSet.cpp:296
(Diff revision 2)
>    // calling into the (potentially-parallel) Servo traversal, where a cache hit
>    // is necessary to avoid a data race when updating the cache.
>    mozilla::Unused << aRoot->OwnerDoc()->GetRootElement();
>  
>    MOZ_ASSERT(!sInServoTraversal);
> -  sInServoTraversal = true;
> +  sInServoTraversal = this;

Now that this is a pointer that could potentially dangle, I think we should manage this with an RAII class to remove footguns with early returns. Also please assert that the pointer is null upon entry.
Attachment #8863200 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863201 [details]
Bug 1356103 - Part 4: Add a mechanism for C++ functions to perform post-Servo traversal tasks.

https://reviewboard.mozilla.org/r/135000/#review138942

Please document the reasoning for the custom class here instead of Runnable. IIUC it's:
* Avoiding the allocation.
* Avoiding the virtual call.
* Making it clear that these are only safe to execute immediately after the traversal, since they hold weak pointers to DOM stuff (with non-threadsafe refcounts).

::: layout/style/ServoStyleSet.cpp:340
(Diff revision 2)
> +  RunPostTraversalTasks();
> +

This would be a nice thing to bundle into the RAII class.
Attachment #8863201 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863203 [details]
Bug 1356103 - Part 6: Make gfxUserFontSet refcounting thread-safe.

https://reviewboard.mozilla.org/r/135004/#review138948
Attachment #8863203 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863204 [details]
Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal.

https://reviewboard.mozilla.org/r/135006/#review138950

::: commit-message-a9f8c:7
(Diff revision 2)
> +work that we do during font loads should drop FontFace objects
> +from the FontFaceSet.  (That only happens under
> +nsIDocument::FlushUserFontSet, which is only called on the
> +main thread.)

For each item that has a weak pointer for a PostTraversalTask, can you add an assertion in its destructor that we're not in the servo traversal? And also add a comment around the PostTraversalTask machinery that any new weak pointers should get the same treatment.

::: layout/style/FontFace.cpp:469
(Diff revision 2)
>        Reject(NS_ERROR_DOM_SYNTAX_ERR);
>      } else {
>        Reject(NS_ERROR_DOM_NETWORK_ERR);

The Asymmetric between calling DoResolve vs Reject here is jarring. Can you rename things somehow to make more sense?

::: layout/style/FontFace.cpp:476
(Diff revision 2)
> +void
> +FontFace::DoResolve()
> +{
> +  AssertIsMainThreadOrServoFontMetricsLocked();
> +
> +  if (ServoStyleSet* ss = ServoStyleSet::Current()) {
> +    ss->AppendTask(PostTraversalTask::ResolveFontFaceLoadedPromise(this));
> +    return;
> +  }
> +
> +  mLoaded->MaybeResolve(this);
> +}
> +
> +void
> +FontFace::DoReject(nsresult aResult)
> +{
> +  AssertIsMainThreadOrServoFontMetricsLocked();
> +
> +  if (ServoStyleSet* ss = ServoStyleSet::Current()) {
> +    ss->AppendTask(PostTraversalTask::RejectFontFaceLoadedPromise(this, aResult));
> +    return;
> +  }
> +
> +  mLoaded->MaybeReject(aResult);
> +}

Please add some comments explaining this whole setup, ideally in ServoBindings.cpp, and reference it here.
Attachment #8863204 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863205 [details]
Bug 1356103 - Part 8: Use PostTraversalTasks to deal with FontFaceSet's Promise and DOM event dispatch during Servo traversal.

https://reviewboard.mozilla.org/r/135008/#review138952

::: layout/style/FontFaceSet.cpp:1494
(Diff revision 2)
> +void
> +FontFaceSet::DispatchCheckLoadingFinishedAfterDelay()
> +{
> +  AssertIsMainThreadOrServoFontMetricsLocked();
> +
> +  if (ServoStyleSet* set = ServoStyleSet::Current()) {
> +    set->AppendTask(PostTraversalTask::DispatchFontFaceSetCheckLoadingFinishedAfterDelay(this));
> +    return;
> +  }

Add some comments here and below about why we can't dispatch the task directly. The refcount threadsafety issue isn't obvious, and it's better to document that in code rather than the commit message.

::: layout/style/PostTraversalTask.h:38
(Diff revision 2)
> +  static PostTraversalTask DispatchLoadingEventAndReplaceReadyPromise(
> +    dom::FontFaceSet* aFontFaceSet)
> +  {
> +    auto task =
> +      PostTraversalTask(Type::DispatchLoadingEventAndReplaceReadyPromise);
> +    task.mTarget = aFontFaceSet;
> +    return task;
> +  }
> +
> +  static PostTraversalTask DispatchFontFaceSetCheckLoadingFinishedAfterDelay(
> +    dom::FontFaceSet* aFontFaceSet)
> +  {
> +    auto task =
> +      PostTraversalTask(Type::DispatchFontFaceSetCheckLoadingFinishedAfterDelay);
> +    task.mTarget = aFontFaceSet;
> +    return task;

Same treatment for these weak pointers as requested before.
Attachment #8863205 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863206 [details]
Bug 1356103 - Part 9: Use a PostTraversalTask to deal with downloadable fonts in gfxUserFontSet.

https://reviewboard.mozilla.org/r/135010/#review138958

I didn't really look at the font-specific stuff, but the PostTraversalTask stuff looks fine.

::: layout/style/PostTraversalTask.cpp:39
(Diff revision 2)
> +    case Type::LoadFontEntry:
> +      static_cast<gfxUserFontEntry*>(mTarget)->ContinueLoad();
> +      break;

Same here about the destructor assert.
Attachment #8863206 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863209 [details]
Bug 1356103 - Part 12: Allow OMT heap writes under Gecko_GetFontMetrics.

https://reviewboard.mozilla.org/r/135016/#review138960
Attachment #8863209 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863208 [details]
Bug 1356103 - Part 11: Adjust text expectations.

https://reviewboard.mozilla.org/r/135014/#review138962
Attachment #8863208 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863207 [details]
Bug 1356103 - Part 10: Re-enable font metrics querying for ch and ex units in Servo traversal.

https://reviewboard.mozilla.org/r/135012/#review138964
Attachment #8863207 - Flags: review?(bobbyholley) → review+
Comment on attachment 8863204 [details]
Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal.

https://reviewboard.mozilla.org/r/135006/#review138950

> For each item that has a weak pointer for a PostTraversalTask, can you add an assertion in its destructor that we're not in the servo traversal? And also add a comment around the PostTraversalTask machinery that any new weak pointers should get the same treatment.

Great idea!
Comment on attachment 8863204 [details]
Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal.

https://reviewboard.mozilla.org/r/135006/#review138950

> The Asymmetric between calling DoResolve vs Reject here is jarring. Can you rename things somehow to make more sense?

Well, the Do* methods are the ones that actually interact with the Promise.  Reject does some other work, so that's why we have it in addition to DoReject.  I could add a Resolve that just calls DoResolve but I don't think it's worth it.
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/784865d234cd
Part 1: Add WeakPtrTraits to allow SupportsWeakPtr classes to opt in to more permissive thread ownership assertions. r=bholley
https://hg.mozilla.org/integration/autoland/rev/559f06e32df3
Part 2: Add functions to assert the Servo font metrics mutex is locked. r=bholley
https://hg.mozilla.org/integration/autoland/rev/1c7831db6b07
Part 3: Make it easy to access the ServoStyleSet currently in traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/529d037f9c33
Part 4: Add a mechanism for C++ functions to perform post-Servo traversal tasks. r=bholley
https://hg.mozilla.org/integration/autoland/rev/31a0881cfb47
Part 5: Allow access to WeakPtr<UnscaledFont> when the Servo font metrics mutex is locked. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/34280baeaabe
Part 6: Make gfxUserFontSet refcounting thread-safe. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c60b4c9cbd83
Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/a03112e1c9d5
Part 8: Use PostTraversalTasks to deal with FontFaceSet's Promise and DOM event dispatch during Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2f383d89184b
Part 9: Use a PostTraversalTask to deal with downloadable fonts in gfxUserFontSet. r=bholley,jfkthame
https://hg.mozilla.org/integration/autoland/rev/7bc3a4861a39
Part 10: Re-enable font metrics querying for ch and ex units in Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/301237c65945
Part 11: Adjust text expectations. r=bholley
https://hg.mozilla.org/integration/autoland/rev/9fb487252c28
Part 12: Allow OMT heap writes under Gecko_GetFontMetrics. r=bholley
backed this out for build bustage at PostTraversalTask.h like https://treeherder.mozilla.org/logviewer.html#?job_id=96547947&repo=autoland&lineNumber=4983
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/034d72052f8c
Part 1: Add WeakPtrTraits to allow SupportsWeakPtr classes to opt in to more permissive thread ownership assertions. r=bholley
https://hg.mozilla.org/integration/autoland/rev/81df1087642e
Part 2: Add functions to assert the Servo font metrics mutex is locked. r=bholley
https://hg.mozilla.org/integration/autoland/rev/8ec2cc2ac751
Part 3: Make it easy to access the ServoStyleSet currently in traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/17ead2243296
Part 4: Add a mechanism for C++ functions to perform post-Servo traversal tasks. r=bholley
https://hg.mozilla.org/integration/autoland/rev/946443516852
Part 5: Allow access to WeakPtr<UnscaledFont> when the Servo font metrics mutex is locked. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/f2ca4b7b166f
Part 6: Make gfxUserFontSet refcounting thread-safe. r=bholley
https://hg.mozilla.org/integration/autoland/rev/0fc9bf9f5294
Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c342d7c71100
Part 8: Use PostTraversalTasks to deal with FontFaceSet's Promise and DOM event dispatch during Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/165c49f2d2d9
Part 9: Use a PostTraversalTask to deal with downloadable fonts in gfxUserFontSet. r=bholley,jfkthame
https://hg.mozilla.org/integration/autoland/rev/e282e719b67e
Part 10: Re-enable font metrics querying for ch and ex units in Servo traversal. r=bholley
https://hg.mozilla.org/integration/autoland/rev/94c059551273
Part 11: Adjust text expectations. r=bholley
https://hg.mozilla.org/integration/autoland/rev/3e84db544489
Part 12: Allow OMT heap writes under Gecko_GetFontMetrics. r=bholley
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f8cb62a8749
Backed out 12 changesets for build bustage at PostTraversalTask.h
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: