test_redundant_font_download.html fails on jemalloc3 builds

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ggp, Assigned: jfkthame)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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!
Flags: needinfo?(jdaggett)
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.
Flags: needinfo?(jdaggett) → needinfo?(jfkthame)
(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.
Flags: needinfo?(jfkthame)
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 ||
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.
Depends on: 704320
Flags: needinfo?(sstamm)
Blocks: 704320
No longer depends on: 704320
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()?
Flags: needinfo?(sstamm)
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?
(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.
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.
Looks like the FontFaceSet already has a reference to the document, so can we simply do this?
Attachment #8546005 - Flags: review?(sstamm)
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.)
Attachment #8546005 - Flags: review?(sstamm) → review+
(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
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/f8466e0caae1
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Duplicate of this bug: 1120077
You need to log in before you can comment on or make changes to this bug.