Bug 1523181 - Don't implicitly flush the user font set. r=heycam, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 27 Jan 2019 16:30:38 +0100
changeset 515808 f8aadf41dcd052cf78272d82fd262094a8e26567
parent 515807 3b7ebdaada3d017ef158294cc3236877c2aa980b
child 515809 f15609e45c76e7186fbcf26eadebd4a75e484264
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam, RyanVM
bugs1523181
milestone66.0
Bug 1523181 - Don't implicitly flush the user font set. r=heycam, a=RyanVM Summary: Flushing it at a bad time can cancel loads whose timer / completion handler is in progress, which makes no sense. Reviewers: jfkthame, jwatt, heycam Tags: #secure-revision Bug #: 1523181 Differential Revision: https://phabricator.services.mozilla.com/D17856
dom/base/Document.cpp
dom/base/Document.h
dom/canvas/CanvasRenderingContext2D.cpp
layout/base/PresShell.cpp
layout/base/nsLayoutUtils.cpp
layout/base/nsLayoutUtils.h
layout/base/nsPresContext.cpp
layout/base/nsPresContext.h
layout/style/GeckoBindings.cpp
layout/style/ServoStyleSet.cpp
uriloader/base/nsDocLoader.cpp
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -1243,17 +1243,16 @@ Document::Document(const char* aContentT
       mHasUnsafeInlineCSP(false),
       mBFCacheDisallowed(false),
       mHasHadDefaultView(false),
       mStyleSheetChangeEventsEnabled(false),
       mIsSrcdocDocument(false),
       mDidDocumentOpen(false),
       mHasDisplayDocument(false),
       mFontFaceSetDirty(true),
-      mGetUserFontSetCalled(false),
       mDidFireDOMContentLoaded(true),
       mHasScrollLinkedEffect(false),
       mFrameRequestCallbacksScheduled(false),
       mIsTopLevelContentDocument(false),
       mIsContentDocument(false),
       mDidCallBeginLoad(false),
       mAllowPaymentRequest(false),
       mEncodingMenuDisabled(false),
@@ -11597,57 +11596,25 @@ nsAutoSyncOperation::~nsAutoSyncOperatio
     doc->SetIsInSyncOperation(false);
   }
   CycleCollectedJSContext* ccjs = CycleCollectedJSContext::Get();
   if (ccjs) {
     ccjs->SetMicroTaskLevel(mMicroTaskLevel);
   }
 }
 
-gfxUserFontSet* Document::GetUserFontSet(bool aFlushUserFontSet) {
-  // We want to initialize the user font set lazily the first time the
-  // user asks for it, rather than building it too early and forcing
-  // rule cascade creation.  Thus we try to enforce the invariant that
-  // we *never* build the user font set until the first call to
-  // GetUserFontSet.  However, once it's been requested, we can't wait
-  // for somebody to call GetUserFontSet in order to rebuild it (see
-  // comments below in MarkUserFontSetDirty for why).
-#ifdef DEBUG
-  bool userFontSetGottenBefore = mGetUserFontSetCalled;
-#endif
-  // Set mGetUserFontSetCalled up front, so that FlushUserFontSet will actually
-  // flush.
-  mGetUserFontSetCalled = true;
-  if (mFontFaceSetDirty && aFlushUserFontSet) {
-    // If this assertion fails, and there have actually been changes to
-    // @font-face rules, then we will call StyleChangeReflow in
-    // FlushUserFontSet.  If we're in the middle of reflow,
-    // that's a bad thing to do, and the caller was responsible for
-    // flushing first.  If we're not (e.g., in frame construction), it's
-    // ok.
-    NS_ASSERTION(!userFontSetGottenBefore || !GetShell() ||
-                     !GetShell()->IsReflowLocked(),
-                 "FlushUserFontSet should have been called first");
-    FlushUserFontSet();
-  }
-
+gfxUserFontSet* Document::GetUserFontSet() {
   if (!mFontFaceSet) {
     return nullptr;
   }
 
   return mFontFaceSet->GetUserFontSet();
 }
 
 void Document::FlushUserFontSet() {
-  if (!mGetUserFontSetCalled) {
-    return;  // No one cares about this font set yet, but we want to be careful
-             // to not unset our mFontFaceSetDirty bit, so when someone really
-             // does we'll create it.
-  }
-
   if (!mFontFaceSetDirty) {
     return;
   }
 
   mFontFaceSetDirty = false;
 
   if (gfxPlatform::GetPlatform()->DownloadableFontsEnabled()) {
     nsTArray<nsFontFaceRuleContainer> rules;
@@ -11674,32 +11641,30 @@ void Document::FlushUserFontSet() {
       if (nsPresContext* presContext = shell->GetPresContext()) {
         presContext->UserFontSetUpdated();
       }
     }
   }
 }
 
 void Document::MarkUserFontSetDirty() {
-  if (!mGetUserFontSetCalled) {
-    // We want to lazily build the user font set the first time it's
-    // requested (so we don't force creation of rule cascades too
-    // early), so don't do anything now.
-    return;
-  }
-
+  if (mFontFaceSetDirty) {
+    return;
+  }
   mFontFaceSetDirty = true;
+  if (nsIPresShell* shell = GetShell()) {
+    shell->EnsureStyleFlush();
+  }
 }
 
 FontFaceSet* Document::Fonts() {
   if (!mFontFaceSet) {
     nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(GetScopeObject());
     mFontFaceSet = new FontFaceSet(window, this);
-    GetUserFontSet();  // this will cause the user font set to be
-                       // created/updated
+    FlushUserFontSet();
   }
   return mFontFaceSet;
 }
 
 void Document::ReportHasScrollLinkedEffect() {
   if (mHasScrollLinkedEffect) {
     // We already did this once for this document, don't do it again.
     return;
--- a/dom/base/Document.h
+++ b/dom/base/Document.h
@@ -3260,17 +3260,17 @@ class Document : public nsINode,
 
     nsWeakPtr weakNode = do_GetWeakReference(node);
 
     if (weakNode) {
       mBlockedTrackingNodes.AppendElement(weakNode);
     }
   }
 
-  gfxUserFontSet* GetUserFontSet(bool aFlushUserFontSet = true);
+  gfxUserFontSet* GetUserFontSet();
   void FlushUserFontSet();
   void MarkUserFontSetDirty();
   mozilla::dom::FontFaceSet* GetFonts() { return mFontFaceSet; }
 
   // FontFaceSource
   mozilla::dom::FontFaceSet* Fonts();
 
   bool DidFireDOMContentLoaded() const { return mDidFireDOMContentLoaded; }
@@ -3986,19 +3986,16 @@ class Document : public nsINode,
   // be a resource document.  Normally this is the same as !!mDisplayDocument,
   // but mDisplayDocument is cleared during Unlink.  mHasDisplayDocument is
   // valid in the document's destructor.
   bool mHasDisplayDocument : 1;
 
   // Is the current mFontFaceSet valid?
   bool mFontFaceSetDirty : 1;
 
-  // Has GetUserFontSet() been called?
-  bool mGetUserFontSetCalled : 1;
-
   // True if we have fired the DOMContentLoaded event, or don't plan to fire one
   // (e.g. we're not being parsed at all).
   bool mDidFireDOMContentLoaded : 1;
 
   // True if ReportHasScrollLinkedEffect() has been called.
   bool mHasScrollLinkedEffect : 1;
 
   // True if we have frame request callbacks scheduled with the refresh driver.
--- a/dom/canvas/CanvasRenderingContext2D.cpp
+++ b/dom/canvas/CanvasRenderingContext2D.cpp
@@ -3481,16 +3481,18 @@ bool CanvasRenderingContext2D::SetFontIn
   // device pixels, to avoid being affected by page zoom. nsFontMetrics will
   // convert nsFont size in app units to device pixels for the font group, so
   // here we first apply to the size the equivalent of a conversion from device
   // pixels to CSS pixels, to adjust for the difference in expectations from
   // other nsFontMetrics clients.
   resizedFont.size =
       (fontStyle->mSize * c->AppUnitsPerDevPixel()) / AppUnitsPerCSSPixel();
 
+  c->Document()->FlushUserFontSet();
+
   nsFontMetrics::Params params;
   params.language = fontStyle->mLanguage;
   params.explicitLanguage = fontStyle->mExplicitLanguage;
   params.userFontSet = c->GetUserFontSet();
   params.textPerf = c->GetTextPerfMetrics();
   RefPtr<nsFontMetrics> metrics =
       c->DeviceContext()->GetMetricsFor(resizedFont, params);
 
@@ -4021,16 +4023,17 @@ nsresult CanvasRenderingContext2D::DrawO
 
   MOZ_ASSERT(!presShell->IsDestroying(),
              "GetCurrentFontStyle() should have returned null if the presshell "
              "is being destroyed");
 
   nsPresContext* presContext = presShell->GetPresContext();
 
   // ensure user font set is up to date
+  presContext->Document()->FlushUserFontSet();
   currentFontStyle->SetUserFontSet(presContext->GetUserFontSet());
 
   if (currentFontStyle->GetStyle()->size == 0.0F) {
     if (aWidth) {
       *aWidth = 0;
     }
     return NS_OK;
   }
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -1166,19 +1166,17 @@ void PresShell::Destroy() {
   gfxTextPerfMetrics* tp;
   if (mPresContext && (tp = mPresContext->GetTextPerfMetrics())) {
     tp->Accumulate();
     if (tp->cumulative.numChars > 0) {
       LogTextPerfStats(tp, this, tp->cumulative, 0.0, eLog_totals, nullptr);
     }
   }
   if (mPresContext) {
-    const bool mayFlushUserFontSet = false;
-    gfxUserFontSet* fs = mPresContext->GetUserFontSet(mayFlushUserFontSet);
-    if (fs) {
+    if (gfxUserFontSet* fs = mPresContext->GetUserFontSet()) {
       uint32_t fontCount;
       uint64_t fontSize;
       fs->GetLoadStatistics(fontCount, fontSize);
       Telemetry::Accumulate(Telemetry::WEBFONT_PER_PAGE, fontCount);
       Telemetry::Accumulate(Telemetry::WEBFONT_SIZE_PER_PAGE,
                             uint32_t(fontSize / 1024));
     } else {
       Telemetry::Accumulate(Telemetry::WEBFONT_PER_PAGE, 0);
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -9564,30 +9564,27 @@ static nsRect ComputeHTMLReferenceRect(n
   return StaticPrefs::layout_css_control_characters_visible()
              ? NS_STYLE_CONTROL_CHARACTER_VISIBILITY_VISIBLE
              : NS_STYLE_CONTROL_CHARACTER_VISIBILITY_HIDDEN;
 }
 
 /* static */
 already_AddRefed<nsFontMetrics> nsLayoutUtils::GetMetricsFor(
     nsPresContext* aPresContext, bool aIsVertical,
-    const nsStyleFont* aStyleFont, nscoord aFontSize, bool aUseUserFontSet,
-    FlushUserFontSet aFlushUserFontSet) {
+    const nsStyleFont* aStyleFont, nscoord aFontSize, bool aUseUserFontSet) {
   nsFont font = aStyleFont->mFont;
   font.size = aFontSize;
   gfxFont::Orientation orientation =
       aIsVertical ? gfxFont::eVertical : gfxFont::eHorizontal;
   nsFontMetrics::Params params;
   params.language = aStyleFont->mLanguage;
   params.explicitLanguage = aStyleFont->mExplicitLanguage;
   params.orientation = orientation;
-  params.userFontSet = aUseUserFontSet
-                           ? aPresContext->GetUserFontSet(aFlushUserFontSet ==
-                                                          FlushUserFontSet::Yes)
-                           : nullptr;
+  params.userFontSet =
+      aUseUserFontSet ? aPresContext->GetUserFontSet() : nullptr;
   params.textPerf = aPresContext->GetTextPerfMetrics();
   return aPresContext->DeviceContext()->GetMetricsFor(font, params);
 }
 
 /* static */ void nsLayoutUtils::FixupNoneGeneric(
     nsFont* aFont, const nsPresContext* aPresContext, uint8_t aGenericFontID,
     const nsFont* aDefaultVariableFont) {
   bool useDocumentFonts =
--- a/layout/base/nsLayoutUtils.h
+++ b/layout/base/nsLayoutUtils.h
@@ -2940,25 +2940,21 @@ class nsLayoutUtils {
 
   static nsPoint ComputeOffsetToUserSpace(nsDisplayListBuilder* aBuilder,
                                           nsIFrame* aFrame);
 
   // Return the default value to be used for -moz-control-character-visibility,
   // from preferences.
   static uint8_t ControlCharVisibilityDefault();
 
-  enum class FlushUserFontSet {
-    Yes,
-    No,
-  };
-
+  // Callers are responsible to ensure the user-font-set is up-to-date if
+  // aUseUserFontSet is true.
   static already_AddRefed<nsFontMetrics> GetMetricsFor(
       nsPresContext* aPresContext, bool aIsVertical,
-      const nsStyleFont* aStyleFont, nscoord aFontSize, bool aUseUserFontSet,
-      FlushUserFontSet aFlushUserFontSet);
+      const nsStyleFont* aStyleFont, nscoord aFontSize, bool aUseUserFontSet);
 
   /**
    * Appropriately add the correct font if we are using DocumentFonts or
    * overriding for XUL
    */
   static void FixupNoneGeneric(nsFont* aFont, const nsPresContext* aPresContext,
                                uint8_t aGenericFontID,
                                const nsFont* aDefaultVariableFont);
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -1856,18 +1856,18 @@ bool nsPresContext::HasAuthorSpecifiedRu
   if (pseudoType == CSSPseudoElementType::InheritingAnonBox ||
       pseudoType == CSSPseudoElementType::NonInheritingAnonBox) {
     return false;
   }
   return Servo_HasAuthorSpecifiedRules(computedStyle, elem, pseudoType,
                                        aRuleTypeMask, UseDocumentColors());
 }
 
-gfxUserFontSet* nsPresContext::GetUserFontSet(bool aFlushUserFontSet) {
-  return mDocument->GetUserFontSet(aFlushUserFontSet);
+gfxUserFontSet* nsPresContext::GetUserFontSet() {
+  return mDocument->GetUserFontSet();
 }
 
 void nsPresContext::UserFontSetUpdated(gfxUserFontEntry* aUpdatedFont) {
   if (!mShell) return;
 
   // Note: this method is called without a font when rules in the userfont set
   // are updated, which may occur during reflow as a result of the lazy
   // initialization of the userfont set. It would be better to avoid a full
--- a/layout/base/nsPresContext.h
+++ b/layout/base/nsPresContext.h
@@ -896,17 +896,17 @@ class nsPresContext : public nsISupports
   }
 
   // This method should be used instead of directly accessing mPaintFlashing,
   // as that value may be out of date when mPaintFlashingInitialized is false.
   bool GetPaintFlashing() const;
 
   bool SuppressingResizeReflow() const { return mSuppressResizeReflow; }
 
-  gfxUserFontSet* GetUserFontSet(bool aFlushUserFontSet = true);
+  gfxUserFontSet* GetUserFontSet();
 
   // Should be called whenever the set of fonts available in the user
   // font set changes (e.g., because a new font loads, or because the
   // user font set is changed and fonts become unavailable).
   void UserFontSetUpdated(gfxUserFontEntry* aUpdatedFont = nullptr);
 
   gfxMissingFontRecorder* MissingFontRecorder() { return mMissingFonts.get(); }
 
--- a/layout/style/GeckoBindings.cpp
+++ b/layout/style/GeckoBindings.cpp
@@ -2053,18 +2053,17 @@ GeckoFontMetrics Gecko_GetFontMetrics(Ra
   // 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 = nsLayoutUtils::GetMetricsFor(
-      presContext, aIsVertical, aFont, aFontSize, aUseUserFontSet,
-      nsLayoutUtils::FlushUserFontSet::No);
+      presContext, aIsVertical, aFont, aFontSize, aUseUserFontSet);
 
   ret.mXSize = fm->XHeight();
   gfxFloat zeroWidth = fm->GetThebesFontGroup()
                            ->GetFirstValidFont()
                            ->GetMetrics(fm->Orientation())
                            .zeroOrAveCharWidth;
   ret.mChSize = NS_round(aPresContext->AppUnitsPerDevPixel() * zeroWidth);
   return ret;
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -346,19 +346,17 @@ void ServoStyleSet::SetAuthorStyleDisabl
   // to rebuild stylist for this change. But we have bug around this, and we
   // may want to rethink how things should work. See bug 1437785.
   SetStylistStyleSheetsDirty();
 }
 
 already_AddRefed<ComputedStyle> ServoStyleSet::ResolveStyleFor(
     Element* aElement, LazyComputeBehavior aMayCompute) {
   if (aMayCompute == LazyComputeBehavior::Allow) {
-    PreTraverseSync();
-    return ResolveStyleLazilyInternal(aElement,
-                                      CSSPseudoElementType::NotPseudo);
+    return ResolveStyleLazily(aElement, CSSPseudoElementType::NotPseudo);
   }
 
   return ResolveServoStyle(*aElement);
 }
 
 const ServoElementSnapshotTable& ServoStyleSet::Snapshots() {
   MOZ_ASSERT(GetPresContext(), "Styling a document without a shell?");
   return GetPresContext()->RestyleManager()->Snapshots();
@@ -373,16 +371,22 @@ void ServoStyleSet::ResolveMappedAttrDec
 }
 
 void ServoStyleSet::PreTraverseSync() {
   // Get the Document's root element to ensure that the cache is valid before
   // calling into the (potentially-parallel) Servo traversal, where a cache hit
   // is necessary to avoid a data race when updating the cache.
   mozilla::Unused << mDocument->GetRootElement();
 
+  // FIXME(emilio): This shouldn't be needed in theory, the call to the same
+  // function in PresShell should do the work, but as it turns out we
+  // ProcessPendingRestyles() twice, and runnables from frames just constructed
+  // can end up doing editing stuff, which adds stylesheets etc...
+  mDocument->FlushUserFontSet();
+
   ResolveMappedAttrDeclarationBlocks();
 
   nsMediaFeatures::InitSystemMetrics();
 
   LookAndFeel::NativeInit();
 
   mDocument->CacheAllKnownLangPrefs();
 
@@ -531,17 +535,16 @@ already_AddRefed<ComputedStyle> ServoSty
   MOZ_ASSERT(computedValues);
   return computedValues.forget();
 }
 
 already_AddRefed<ComputedStyle> ServoStyleSet::ResolveStyleLazily(
     Element* aElement, CSSPseudoElementType aPseudoType,
     StyleRuleInclusion aRuleInclusion) {
   PreTraverseSync();
-
   return ResolveStyleLazilyInternal(aElement, aPseudoType, aRuleInclusion);
 }
 
 already_AddRefed<ComputedStyle>
 ServoStyleSet::ResolveInheritingAnonymousBoxStyle(
     nsAtom* aPseudoTag, ComputedStyle* aParentContext) {
   MOZ_ASSERT(nsCSSAnonBoxes::IsAnonBox(aPseudoTag) &&
              !nsCSSAnonBoxes::IsNonInheritingAnonBox(aPseudoTag));
--- a/uriloader/base/nsDocLoader.cpp
+++ b/uriloader/base/nsDocLoader.cpp
@@ -634,18 +634,18 @@ void nsDocLoader::DocLoaderIsEmpty(bool 
       nsCOMPtr<mozilla::dom::Document> doc =
           do_GetInterface(GetAsSupports(this));
       if (doc) {
         // We start loads from style resolution, so we need to flush out style
         // no matter what.  If we have user fonts, we also need to flush layout,
         // since the reflow is what starts font loads.
         mozilla::FlushType flushType = mozilla::FlushType::Style;
         // Be safe in case this presshell is in teardown now
-        nsPresContext* presContext = doc->GetPresContext();
-        if (presContext && presContext->GetUserFontSet()) {
+        doc->FlushUserFontSet();
+        if (doc->GetUserFontSet()) {
           flushType = mozilla::FlushType::Layout;
         }
         mDontFlushLayout = mIsFlushingLayout = true;
         doc->FlushPendingNotifications(flushType);
         mDontFlushLayout = mIsFlushingLayout = false;
       }
     }