Closed Bug 1107694 Opened 6 years ago Closed 6 years ago
_redundant _font _download .html fails on jemalloc3 builds
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) → 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
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()?
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.