Bug 1523181 - Don't implicitly flush the user font set.
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 27 Jan 2019 16:30:38 +0100
changeset 456694 6882b9791212ce241473249f4456c5b4f07fcba1
parent 456693 18fc86b209a417019f745c7417247e9553463933
child 456695 6ac582b8f626a77e924d0465c12793a6945f09bf
push id111694
push useremilio@crisal.io
push dateTue, 05 Feb 2019 12:30:32 +0000
treeherdermozilla-inbound@6882b9791212 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1523181
milestone67.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 1523181 - Don't implicitly flush the user font set. 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
@@ -1241,17 +1241,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),
@@ -11596,57 +11595,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;
@@ -11673,32 +11640,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
@@ -3334,17 +3334,17 @@ class Document : public nsINode,
 
     nsWeakPtr weakNode = do_GetWeakReference(node);
 
     if (weakNode) {
       mBlockedNodesByClassifier.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; }
@@ -4060,19 +4060,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
@@ -1152,19 +1152,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
@@ -9578,30 +9578,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
@@ -1859,18 +1859,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
@@ -2043,18 +2043,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
@@ -635,18 +635,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;
       }
     }