Guilherme Gonçalves [:ggp] Reporter
	Description • 10 years ago

STR:
1- Build with MOZ_JEMALLOC3=1 on your mozconfig
2- Try to run the test, preferably more than once:
./mach mochitest-plain --repeat 3 layout/style/test/test_redundant_font_download.html

The test will fail with 'should NOT have re-requested the font - got init;font;image;font, expected init;font;image'

I first reproduced this in a Try run:https://treeherder.mozilla.org/#/jobs?repo=try&revision=fada2ab35e79 . Though that contains a bunch of other patches, I did manage to reproduce on latest m-c locally.

	Guilherme Gonçalves [:ggp] Reporter
	Comment 1 • 10 years ago

Hi John. We're (informally) targeting the end of December to enable jemalloc3 by default, and this bug could potentially hold us back. Does this look like a severe bug to you, or should it be easily fixable? Thanks!

	John Daggett (:jtd)
	Comment 2 • 10 years ago

Hmmm, is this perhaps a framework issue? If you run this without '--repeat 3' does the failure occur? This test is uses server-side javascript code to evaluate whether an additional load occurs within *one* run of the test. Seems like that server-side state should be reset with each pass. Or we need to tweak the logic in that test to essentially clear out previous state at the start of each test.

needinfo'ing jonathan who wrote this test.

	Jonathan Kew [:jfkthame] Assignee
	Comment 3 • 10 years ago

(In reply to John Daggett (:jtd) from comment #2)
> Hmmm, is this perhaps a framework issue? If you run this without '--repeat
> 3' does the failure occur? 

My understanding is that it can happen even without the repeat (e.g. see the try run mentioned in comment #0); using --repeat just seems to be a way to reproduce much more reliably on a local machine.

> This test is uses server-side javascript code to
> evaluate whether an additional load occurs within *one* run of the test.
> Seems like that server-side state should be reset with each pass. Or we need
> to tweak the logic in that test to essentially clear out previous state at
> the start of each test.

The failure quoted

  "should NOT have re-requested the font - got init;font;image;font, expected init;font;image"

indicates that the state was initialized as expected: it starts with 'init'; it doesn't have pre-existing state and *then* a new 'init' entry.

(The .sjs server was deliberately written such that the 'init' call replaces any pre-existing state.)

So AFAICS, the test is showing a real behavior regression here: somehow, the change to jemalloc is altering the @font-face rule management/caching/something. I wonder if there's some kind of use of an uninitialized variable, or a use-after-free, or something like that; and the change of allocator is affecting the likely content of whatever is involved there. But that's only a guess. Maybe running the text under valgrind would show something.

	Mike Hommey [:glandium]
	Comment 4 • 10 years ago

Valgrind pointed in the right direction, although not in a very obvious way:

==4337== Conditional jump or move depends on uninitialised value(s)
==4337==    at 0x1198D2E2: operator==(gfxFontFaceSrc const&, gfxFontFaceSrc const&) (gfxUserFontSet.h:76)
==4337==    by 0x1198CBA4: bool nsTArray_Impl<gfxFontFaceSrc, nsTArrayInfallibleAllocator>::operator==<nsTArrayInfallibleAllocator>(nsTArray_Impl<gfxFontFaceSrc, nsTArrayInfallibleAllocator> const&) const (nsTArray.h:869)
==4337==    by 0x11988E88: gfxUserFontEntry::Matches(nsTArray<gfxFontFaceSrc> const&, unsigned int, int, unsigned int, nsTArray<gfxFontFeature> const&, unsigned int, gfxSparseBitSet*) (gfxUserFontSet.cpp:166)
==4337==    by 0x1198A920: gfxUserFontSet::FindExistingUserFontEntry(gfxUserFontFamily*, nsTArray<gfxFontFaceSrc> const&, unsigned int, int, unsigned int, nsTArray<gfxFontFeature> const&, unsigned int, gfxSparseBitSet*) (gfxUserFontSet.cpp:846)
==4337==    by 0x1198A723: gfxUserFontSet::FindOrCreateUserFontEntry(nsAString_internal const&, nsTArray<gfxFontFaceSrc> const&, unsigned int, int, unsigned int, nsTArray<gfxFontFeature> const&, unsigned int, gfxSparseBitSet*) (gfxUserFontSet.cpp:778)
==4337==    by 0x125ED3CD: mozilla::dom::FontFaceSet::FindOrCreateUserFontEntryFromFontFace(nsAString_internal const&, mozilla::dom::FontFace*, unsigned char) (FontFaceSet.cpp:983)
==4337==    by 0x125ECB5D: mozilla::dom::FontFaceSet::InsertRuleFontFace(mozilla::dom::FontFace*, unsigned char, nsTArray<mozilla::dom::FontFaceSet::FontFaceRecord>&, bool&) (FontFaceSet.cpp:739)
==4337==    by 0x125EC80B: mozilla::dom::FontFaceSet::UpdateRules(nsTArray<nsFontFaceRuleContainer> const&) (FontFaceSet.cpp:556)
==4337==    by 0x126F420A: nsPresContext::FlushUserFontSet() (nsPresContext.cpp:2081)
==4337==    by 0x126F410B: nsPresContext::GetUserFontSetInternal() (nsPresContext.cpp:2041)
==4337==    by 0x126EA70F: GetUserFontSet (nsPresContext.h:866)
==4337==    by 0x126EA70F: nsLayoutUtils::GetFontMetricsForStyleContext(nsStyleContext*, nsFontMetrics**, float) (nsLayoutUtils.cpp:3768)
==4337==    by 0x12755F05: ComputeLineHeight (nsHTMLReflowState.cpp:2558)
==4337==    by 0x12755F05: nsHTMLReflowState::CalcLineHeight(nsIContent*, nsStyleContext*, int, float) (nsHTMLReflowState.cpp:2584)
==4337==

That points to the mReferrer test in operator==(const gfxFontFaceSrc&, const gfxFontFaceSrc&), but that's actually a red herring. The real non-initialized bit is mReferrerPolicy.

The following patch seems to fix it for me, but I don't know what the right value should be for this referrer policy:

diff --git a/layout/style/FontFaceSet.cpp b/layout/style/FontFaceSet.cpp
index 385b9c7..626db20 100644
--- a/layout/style/FontFaceSet.cpp
+++ b/layout/style/FontFaceSet.cpp
@@ -915,16 +915,17 @@ FontFaceSet::FindOrCreateUserFontEntryFromFontFace(const nsAString& aFamilyName,
           face->mSourceType = gfxFontFaceSrc::eSourceType_Local;
           face->mURI = nullptr;
           face->mFormatFlags = 0;
           break;
         case eCSSUnit_URL:
           face->mSourceType = gfxFontFaceSrc::eSourceType_URL;
           face->mURI = val.GetURLValue();
           face->mReferrer = val.GetURLStructValue()->mReferrer;
+          face->mReferrerPolicy = net::RP_No_Referrer;
           face->mOriginPrincipal = val.GetURLStructValue()->mOriginPrincipal;
           NS_ASSERTION(face->mOriginPrincipal, "null origin principal in @font-face rule");
 
           // agent and user stylesheets are treated slightly differently,
           // the same-site origin check and access control headers are
           // enforced against the sheet principal rather than the document
           // principal to allow user stylesheets to include @font-face rules
           face->mUseOriginPrincipal = (aSheetType == nsStyleSet::eUserSheet ||

	Jonathan Kew [:jfkthame] Assignee
	Comment 5 • 10 years ago

That field was added in patch 3 of bug 704320, which points to this being a regression from that bug. Perhpas Sid could confirm what the appropriate value would be.

	Sid Stamm [:geekboy or :sstamm]
	Comment 6 • 9 years ago

Hm.  We probably want to do the same thing as we did in gfxUserFontSet.h, I think (though I admit I'm not very familiar with this part of the code).
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.h#62

The referrer policy should be set by the triggering context; for example, the stylesheet has a referrer policy it gets from a document, and that referrer policy should be used for loading fonts.

Elsewhere in the file we get the ReferrerPolicy out of the PresShell:
http://mxr.mozilla.org/mozilla-central/source/layout/style/FontFaceSet.cpp#429
Maybe you could grab this from mPresContext->PresShell()->GetDocument()->GetReferrerPolicy()?

	Guilherme Gonçalves [:ggp] Reporter
	Comment 7 • 9 years ago

I can't reproduce this anymore on latest m-c, neither locally, nor on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8548da53152a . Unless someone else can, maybe we can call this WORKSFORME?

	Jonathan Kew [:jfkthame] Assignee
	Comment 8 • 9 years ago

(In reply to Guilherme Gonçalves [:ggp] from comment #7)
> I can't reproduce this anymore on latest m-c, neither locally, nor on Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8548da53152a .
> Unless someone else can, maybe we can call this WORKSFORME?

No, I don't think we should; it'll come back to bite us some day. Comment 4 makes it clear there's an uninitialized mReferrerPolicy field involved here, so the behavior may change unpredictably with unrelated changes that affect memory allocation, layout, etc.; we need to fix that.

	Guilherme Gonçalves [:ggp] Reporter
	Comment 9 • 9 years ago

Oh, I was wondering if it could have been unintentionally fixed in the past week. But yes, if the access to uninitialized memory is still there, it should definitely be addressed.

	Jonathan Kew [:jfkthame] Assignee
	Comment 10 • 9 years ago

Looks like the FontFaceSet already has a reference to the document, so can we simply do this?

	Sid Stamm [:geekboy or :sstamm]
	Comment 11 • 9 years ago

Comment on attachment 8546005 [details] [diff] [review]
Make sure to set the referrer-policy for font-face source URLs

Review of attachment 8546005 [details] [diff] [review]:
-----------------------------------------------------------------

That should be sufficient, so long as mDocument is never unset.  It doesn't make sense to me to have the font without a document, but maybe there's a way?  (If there is a way to have a null mDocument, this could be a problem.)

	Jonathan Kew [:jfkthame] Assignee
	Comment 12 • 9 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)

> That should be sufficient, so long as mDocument is never unset.  It doesn't
> make sense to me to have the font without a document, but maybe there's a
> way?  (If there is a way to have a null mDocument, this could be a problem.)

AFAICS, it should always be set; it only gets cleared during destruction of the FontFaceSet, at which point this should no longer get called.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8466e0caae1

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f8466e0caae1