Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal. r=bholley
☠☠ backed out by 8f8cb62a8749 ☠ ☠
authorCameron McCormack <cam@mcc.id.au>
Sun, 30 Apr 2017 14:51:29 +0800
changeset 404585 c60b4c9cbd83e774eca3014e1e0d78f67b055b62
parent 404584 34280baeaabe27be2c7d587b6e1875d260bdbcdf
child 404586 a03112e1c9d5597e91b48d94a49d62890a33d298
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1356103
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1356103 - Part 7: Use PostTraversalTasks to deal with FontFace's Promise during Servo traversal. r=bholley The PostTraversalTask does not take a strong reference to the FontFace object, since as a DOM object, we can't call AddRef/Release on it from the Servo style worker threads. The FontFace objects that we encounter are all held on to by the FontFaceSet, and none of the 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.) MozReview-Commit-ID: 5CdtGQYC9aL
layout/style/FontFace.cpp
layout/style/FontFace.h
layout/style/PostTraversalTask.cpp
layout/style/PostTraversalTask.h
layout/style/ServoBindings.cpp
--- a/layout/style/FontFace.cpp
+++ b/layout/style/FontFace.cpp
@@ -7,16 +7,18 @@
 
 #include <algorithm>
 #include "mozilla/dom/FontFaceBinding.h"
 #include "mozilla/dom/FontFaceSet.h"
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/TypedArray.h"
 #include "mozilla/dom/UnionTypes.h"
 #include "mozilla/CycleCollectedJSContext.h"
+#include "mozilla/ServoStyleSet.h"
+#include "mozilla/ServoUtils.h"
 #include "nsCSSFontFaceRule.h"
 #include "nsCSSParser.h"
 #include "nsIDocument.h"
 #include "nsStyleUtil.h"
 
 namespace mozilla {
 namespace dom {
 
@@ -107,16 +109,20 @@ FontFace::FontFace(nsISupports* aParent,
   , mSourceBufferLength(0)
   , mFontFaceSet(aFontFaceSet)
   , mInFontFaceSet(false)
 {
 }
 
 FontFace::~FontFace()
 {
+  // Assert that we don't drop any FontFace objects during a Servo traversal,
+  // since PostTraversalTask objects can hold raw pointers to FontFaces.
+  MOZ_ASSERT(!ServoStyleSet::IsInServoTraversal());
+
   SetUserFontEntry(nullptr);
 
   if (mSourceBuffer) {
     free(mSourceBuffer);
   }
 }
 
 JSObject*
@@ -352,16 +358,18 @@ FontFaceLoadStatus
 FontFace::Status()
 {
   return mStatus;
 }
 
 Promise*
 FontFace::Load(ErrorResult& aRv)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
   mFontFaceSet->FlushUserFontSet();
 
   EnsurePromise();
 
   if (!mLoaded) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
@@ -409,31 +417,35 @@ FontFace::DoLoad()
     return;
   }
   mUserFontEntry->Load();
 }
 
 Promise*
 FontFace::GetLoaded(ErrorResult& aRv)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
   mFontFaceSet->FlushUserFontSet();
 
   EnsurePromise();
 
   if (!mLoaded) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   return mLoaded;
 }
 
 void
 FontFace::SetStatus(FontFaceLoadStatus aStatus)
 {
+  AssertIsMainThreadOrServoFontMetricsLocked();
+
   if (mStatus == aStatus) {
     return;
   }
 
   if (aStatus < mStatus) {
     // We're being asked to go backwards in status!  Normally, this shouldn't
     // happen.  But it can if the FontFace had a user font entry that had
     // loaded, but then was given a new one by FontFaceSet::InsertRuleFontFace
@@ -449,27 +461,55 @@ FontFace::SetStatus(FontFaceLoadStatus a
   }
 
   for (FontFaceSet* otherSet : mOtherFontFaceSets) {
     otherSet->OnFontFaceStatusChanged(this);
   }
 
   if (mStatus == FontFaceLoadStatus::Loaded) {
     if (mLoaded) {
-      mLoaded->MaybeResolve(this);
+      DoResolve();
     }
   } else if (mStatus == FontFaceLoadStatus::Error) {
     if (mSourceType == eSourceType_Buffer) {
       Reject(NS_ERROR_DOM_SYNTAX_ERR);
     } else {
       Reject(NS_ERROR_DOM_NETWORK_ERR);
     }
   }
 }
 
+void
+FontFace::DoResolve()
+{
+  AssertIsMainThreadOrServoFontMetricsLocked();
+
+  if (ServoStyleSet* ss = ServoStyleSet::Current()) {
+    // See comments in Gecko_GetFontMetrics.
+    ss->AppendTask(PostTraversalTask::ResolveFontFaceLoadedPromise(this));
+    return;
+  }
+
+  mLoaded->MaybeResolve(this);
+}
+
+void
+FontFace::DoReject(nsresult aResult)
+{
+  AssertIsMainThreadOrServoFontMetricsLocked();
+
+  if (ServoStyleSet* ss = ServoStyleSet::Current()) {
+    // See comments in Gecko_GetFontMetrics.
+    ss->AppendTask(PostTraversalTask::RejectFontFaceLoadedPromise(this, aResult));
+    return;
+  }
+
+  mLoaded->MaybeReject(aResult);
+}
+
 bool
 FontFace::ParseDescriptor(nsCSSFontDesc aDescID,
                           const nsAString& aString,
                           nsCSSValue& aResult)
 {
   nsCSSParser parser;
 
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mParent);
@@ -727,26 +767,30 @@ FontFace::RemoveFontFaceSet(FontFaceSet*
   } else {
     mOtherFontFaceSets.RemoveElement(aFontFaceSet);
   }
 }
 
 void
 FontFace::Reject(nsresult aResult)
 {
+  AssertIsMainThreadOrServoFontMetricsLocked();
+
   if (mLoaded) {
-    mLoaded->MaybeReject(aResult);
+    DoReject(aResult);
   } else if (mLoadedRejection == NS_OK) {
     mLoadedRejection = aResult;
   }
 }
 
 void
 FontFace::EnsurePromise()
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
   if (mLoaded) {
     return;
   }
 
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mParent);
 
   // If the pref is not set, don't create the Promise (which the page wouldn't
   // be able to get to anyway) as it causes the window.FontFace constructor
--- a/layout/style/FontFace.h
+++ b/layout/style/FontFace.h
@@ -13,31 +13,33 @@
 #include "nsCSSValue.h"
 #include "nsWrapperCache.h"
 
 class gfxFontFaceBufferSource;
 class nsCSSFontFaceRule;
 
 namespace mozilla {
 struct CSSFontFaceDescriptors;
+class PostTraversalTask;
 namespace dom {
 class FontFaceBufferSource;
 struct FontFaceDescriptors;
 class FontFaceSet;
 class Promise;
 class StringOrArrayBufferOrArrayBufferView;
 } // namespace dom
 } // namespace mozilla
 
 namespace mozilla {
 namespace dom {
 
 class FontFace final : public nsISupports,
                        public nsWrapperCache
 {
+  friend class mozilla::PostTraversalTask;
   friend class mozilla::dom::FontFaceBufferSource;
   friend class Entry;
 
 public:
   class Entry final : public gfxUserFontEntry {
     friend class FontFace;
 
   public:
@@ -202,16 +204,19 @@ private:
   // Acts like mLoaded->MaybeReject(aResult), except it doesn't create mLoaded
   // if it doesn't already exist.
   void Reject(nsresult aResult);
 
   // Creates mLoaded if it doesn't already exist. It may immediately resolve or
   // reject mLoaded based on mStatus and mLoadedRejection.
   void EnsurePromise();
 
+  void DoResolve();
+  void DoReject(nsresult aResult);
+
   nsCOMPtr<nsISupports> mParent;
 
   // A Promise that is fulfilled once the font represented by this FontFace is
   // loaded, and is rejected if the load fails. This promise is created lazily
   // when JS asks for it.
   RefPtr<mozilla::dom::Promise> mLoaded;
 
   // Saves the rejection code for mLoaded if mLoaded hasn't been created yet.
--- a/layout/style/PostTraversalTask.cpp
+++ b/layout/style/PostTraversalTask.cpp
@@ -1,16 +1,29 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "PostTraversalTask.h"
 
+#include "mozilla/dom/FontFace.h"
+
 namespace mozilla {
 
+using namespace dom;
+
 void
 PostTraversalTask::Run()
 {
+  switch (mType) {
+    case Type::ResolveFontFaceLoadedPromise:
+      static_cast<FontFace*>(mTarget)->DoResolve();
+      break;
+
+    case Type::RejectFontFaceLoadedPromise:
+      static_cast<FontFace*>(mTarget)->DoReject(mResult);
+      break;
+  }
 }
 
 } // namespace mozilla
--- a/layout/style/PostTraversalTask.h
+++ b/layout/style/PostTraversalTask.h
@@ -5,41 +5,73 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_PostTraversalTask_h
 #define mozilla_PostTraversalTask_h
 
 /* a task to be performed immediately after a Servo traversal */
 
 namespace mozilla {
+namespace dom {
+class FontFace;
+} // namespace dom
+} // namespace mozilla
+
+namespace mozilla {
 
 /**
  * A PostTraversalTask is a task to be performed immediately after a Servo
  * traversal.  There are just a few tasks we need to perform, so we use this
  * class rather than Runnables, to avoid virtual calls and some allocations.
  *
  * A PostTraversalTask is only safe to run immediately after the Servo
  * traversal, since it can hold raw pointers to DOM objects.
  */
 class PostTraversalTask
 {
 public:
+  static PostTraversalTask ResolveFontFaceLoadedPromise(dom::FontFace* aFontFace)
+  {
+    auto task = PostTraversalTask(Type::ResolveFontFaceLoadedPromise);
+    task.mTarget = aFontFace;
+    return task;
+  }
+
+  static PostTraversalTask RejectFontFaceLoadedPromise(dom::FontFace* aFontFace,
+                                                       nsresult aResult)
+  {
+    auto task = PostTraversalTask(Type::ResolveFontFaceLoadedPromise);
+    task.mTarget = aFontFace;
+    task.mResult = aResult;
+    return task;
+  }
+
   void Run();
 
 private:
+  // For any new raw pointer type that we need to store in a PostTraversalTask,
+  // please add an assertion that class' destructor that we are not in a Servo
+  // traversal, to protect against the possibility of having dangling pointers.
   enum class Type
   {
-    Dummy,
+    // mTarget (FontFace*)
+    ResolveFontFaceLoadedPromise,
+
+    // mTarget (FontFace*)
+    // mResult
+    RejectFontFaceLoadedPromise,
   };
 
   PostTraversalTask(Type aType)
     : mType(aType)
     , mTarget(nullptr)
+    , mResult(NS_OK)
   {
   }
 
   Type mType;
   void* mTarget;
+  nsresult mResult;
 };
 
 } // namespace mozilla
 
 #endif // mozilla_PostTraversalTask_h
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -1841,18 +1841,28 @@ Gecko_GetFontMetrics(RawGeckoPresContext
                      nscoord aFontSize,
                      bool aUseUserFontSet)
 {
   // This function is still unsafe due to frobbing DOM and network
   // off main thread. We currently disable it in Servo, see bug 1356105
   MOZ_ASSERT(NS_IsMainThread());
   MutexAutoLock lock(*sServoFontMetricsLock);
   GeckoFontMetrics ret;
-  // Safe because we are locked, and this function is only
-  // ever called from Servo parallel traversal or the main thread
+
+  // Getting font metrics can require some main thread only work to be
+  // done, such as work that needs to touch non-threadsafe refcounted
+  // objects (like the DOM FontFace/FontFaceSet objects), network loads, etc.
+  //
+  // To handle this work, font code checks whether we are in a Servo traversal
+  // and if so, appends PostTraversalTasks to the current ServoStyleSet
+  // to be performed immediately after the traversal is finished.  This
+  // works well for starting downloadable font loads, since we don't have
+  // those fonts available to get metrics for anyway.  Platform fonts and
+  // ArrayBuffer-backed FontFace objects are handled synchronously.
+
   nsPresContext* presContext = const_cast<nsPresContext*>(aPresContext);
   presContext->SetUsesExChUnits(true);
   RefPtr<nsFontMetrics> fm = nsRuleNode::GetMetricsFor(presContext, aIsVertical,
                                                        aFont, aFontSize,
                                                        aUseUserFontSet);
   ret.mXSize = fm->XHeight();
   gfxFloat zeroWidth = fm->GetThebesFontGroup()->GetFirstValidFont()->
                            GetMetrics(fm->Orientation()).zeroOrAveCharWidth;