From 23932b1a58f528352eae444f780da4fb9f62e3ff Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Thu, 1 Sep 2016 08:05:43 -0400 Subject: [PATCH] Backed out 5 changesets (bug 1298768, bug 1297963) for causing widespread mochitest-bc failures. Backed out changeset dedd56fa5c54 (bug 1297963) Backed out changeset bc1ac59cfe8f (bug 1297963) Backed out changeset a2e337d5aa02 (bug 1297963) Backed out changeset e73da71408a3 (bug 1297963) Backed out changeset 56f8bca8f8e8 (bug 1298768) CLOSED TREE --- dom/base/nsAttrValue.cpp | 4 +- dom/html/nsGenericHTMLElement.cpp | 2 +- layout/base/nsCSSFrameConstructor.cpp | 12 ++--- layout/style/StyleAnimationValue.cpp | 6 +-- layout/style/nsCSSValue.cpp | 66 +++++++++++++-------------- layout/style/nsCSSValue.h | 35 ++++++-------- layout/style/nsStyleStruct.cpp | 8 ++-- 7 files changed, 59 insertions(+), 74 deletions(-) diff --git a/dom/base/nsAttrValue.cpp b/dom/base/nsAttrValue.cpp index 98b479fb7ce4..d3469dc01746 100644 --- a/dom/base/nsAttrValue.cpp +++ b/dom/base/nsAttrValue.cpp @@ -1735,8 +1735,8 @@ nsAttrValue::LoadImage(nsIDocument* aDocument) MiscContainer* cont = GetMiscContainer(); mozilla::css::URLValue* url = cont->mValue.mURL; mozilla::css::ImageValue* image = - new css::ImageValue(url->GetURI(), url->mString, url->mBaseURI, - url->mReferrer, url->mOriginPrincipal, aDocument); + new css::ImageValue(url->GetURI(), url->mString, url->mReferrer, + url->mOriginPrincipal, aDocument); NS_ADDREF(image); cont->mValue.mImage = image; diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp index 268379a5171c..05a94e90416d 100644 --- a/dom/html/nsGenericHTMLElement.cpp +++ b/dom/html/nsGenericHTMLElement.cpp @@ -981,7 +981,7 @@ nsGenericHTMLElement::ParseBackgroundAttribute(int32_t aNamespaceID, } mozilla::css::URLValue *url = - new mozilla::css::URLValue(uri, buffer, baseURI, doc->GetDocumentURI(), + new mozilla::css::URLValue(uri, buffer, doc->GetDocumentURI(), NodePrincipal()); aResult.SetTo(url, &aValue); return true; diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp index e349fb8b120f..2c176dfc7b35 100644 --- a/layout/base/nsCSSFrameConstructor.cpp +++ b/layout/base/nsCSSFrameConstructor.cpp @@ -9175,12 +9175,11 @@ nsCSSFrameConstructor::CaptureStateForFramesOf(nsIContent* aContent, } } -static bool -DefinitelyEqualURIsAndPrincipal(mozilla::css::URLValue* aURI1, - mozilla::css::URLValue* aURI2) +static bool EqualURIs(mozilla::css::URLValue *aURI1, + mozilla::css::URLValue *aURI2) { - return aURI1 == aURI2 || - (aURI1 && aURI2 && aURI1->DefinitelyEqualURIsAndPrincipal(*aURI2)); + return aURI1 == aURI2 || // handle null==null, and optimize + (aURI1 && aURI2 && aURI1->URIEquals(*aURI2)); } nsStyleContext* @@ -9216,8 +9215,7 @@ nsCSSFrameConstructor::MaybeRecreateFramesForElement(Element* aElement) return newContext; } const nsStyleDisplay* oldDisp = oldContext->PeekStyleDisplay(); - if (oldDisp && - DefinitelyEqualURIsAndPrincipal(disp->mBinding, oldDisp->mBinding)) { + if (oldDisp && EqualURIs(disp->mBinding, oldDisp->mBinding)) { return newContext; } } diff --git a/layout/style/StyleAnimationValue.cpp b/layout/style/StyleAnimationValue.cpp index 92f4350ba7b4..ffcf558f18a8 100644 --- a/layout/style/StyleAnimationValue.cpp +++ b/layout/style/StyleAnimationValue.cpp @@ -291,13 +291,9 @@ FragmentOrURLToURLValue(FragmentOrURL* aUrl, nsIDocument* aDoc) nsString path; aUrl->GetSourceString(path); RefPtr uriStringBuffer = nsCSSValue::BufferFromString(path); - // XXXheycam We should store URLValue objects inside FragmentOrURLs so that - // we can extract the original base URI, referrer and principal to pass in - // here. RefPtr result = new mozilla::css::URLValue(aUrl->GetSourceURL(), uriStringBuffer, - aUrl->GetSourceURL(), aDoc->GetDocumentURI(), - aDoc->NodePrincipal()); + aDoc->GetDocumentURI(), aDoc->NodePrincipal()); return result.forget(); } diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp index 0fd8a6b5e2a3..3496c746b396 100644 --- a/layout/style/nsCSSValue.cpp +++ b/layout/style/nsCSSValue.cpp @@ -761,7 +761,6 @@ void nsCSSValue::StartImageLoad(nsIDocument* aDocument) const mozilla::css::ImageValue* image = new mozilla::css::ImageValue(mValue.mURL->GetURI(), mValue.mURL->mString, - mValue.mURL->mBaseURI, mValue.mURL->mReferrer, mValue.mURL->mOriginPrincipal, aDocument); @@ -2593,21 +2592,17 @@ nsCSSValue::Array::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) cons css::URLValueData::URLValueData(already_AddRefed> aURI, nsStringBuffer* aString, - already_AddRefed> aBaseURI, already_AddRefed> aReferrer, already_AddRefed> aOriginPrincipal) : mURI(Move(aURI)) - , mBaseURI(Move(aBaseURI)) , mString(aString) , mReferrer(Move(aReferrer)) , mOriginPrincipal(Move(aOriginPrincipal)) , mURIResolved(true) , mLocalURLFlag(IsLocalRefURL(aString)) { - MOZ_ASSERT(mString); - MOZ_ASSERT(mBaseURI); - MOZ_ASSERT(mOriginPrincipal); + MOZ_ASSERT(mOriginPrincipal, "Must have an origin principal"); } css::URLValueData::URLValueData(nsStringBuffer* aString, @@ -2615,16 +2610,14 @@ css::URLValueData::URLValueData(nsStringBuffer* aString, already_AddRefed> aReferrer, already_AddRefed> aOriginPrincipal) - : mBaseURI(Move(aBaseURI)) + : mURI(Move(aBaseURI)) , mString(aString) , mReferrer(Move(aReferrer)) , mOriginPrincipal(Move(aOriginPrincipal)) , mURIResolved(false) , mLocalURLFlag(IsLocalRefURL(aString)) { - MOZ_ASSERT(aString); - MOZ_ASSERT(mBaseURI); - MOZ_ASSERT(mOriginPrincipal); + MOZ_ASSERT(mOriginPrincipal, "Must have an origin principal"); } bool @@ -2640,44 +2633,50 @@ css::URLValueData::operator==(const URLValueData& aOther) const (mURI && aOther.mURI && NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) && eq)) && - (mBaseURI == aOther.mBaseURI || - (NS_SUCCEEDED(self.mBaseURI.get()->Equals(other.mBaseURI.get(), &eq)) && - eq)) && (mOriginPrincipal == aOther.mOriginPrincipal || - self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())) && - mLocalURLFlag == aOther.mLocalURLFlag; + self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())); } bool -css::URLValueData::DefinitelyEqualURIs(const URLValueData& aOther) const +css::URLValueData::MaybeUnresolvedURIEquals(const URLValueData& aOther) const { - return mBaseURI == aOther.mBaseURI && - (mString == aOther.mString || - NS_strcmp(nsCSSValue::GetBufferValue(mString), - nsCSSValue::GetBufferValue(aOther.mString))); + if (!mURIResolved || !aOther.mURIResolved) { + return false; + } + + return URIEquals(aOther); } bool -css::URLValueData::DefinitelyEqualURIsAndPrincipal( - const URLValueData& aOther) const +css::URLValueData::URIEquals(const URLValueData& aOther) const { - return mOriginPrincipal == aOther.mOriginPrincipal && - DefinitelyEqualURIs(aOther); + MOZ_ASSERT(mURIResolved && aOther.mURIResolved, + "How do you know the URIs aren't null?"); + bool eq; + // Cast away const so we can call nsIPrincipal::Equals. + auto& self = *const_cast(this); + auto& other = const_cast(aOther); + // Worth comparing GetURI() to aOther.GetURI() and mOriginPrincipal to + // aOther.mOriginPrincipal, because in the (probably common) case when this + // value was one of the ones that in fact did not change this will be our + // fast path to equality + return (mURI == aOther.mURI || + (NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) && eq)) && + (mOriginPrincipal == aOther.mOriginPrincipal || + self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())); } nsIURI* css::URLValueData::GetURI() const { - MOZ_ASSERT(NS_IsMainThread()); - if (!mURIResolved) { - MOZ_ASSERT(!mURI); + mURIResolved = true; + // Be careful to not null out mURI before we've passed it as the base URI nsCOMPtr newURI; NS_NewURI(getter_AddRefs(newURI), NS_ConvertUTF16toUTF8(nsCSSValue::GetBufferValue(mString)), - nullptr, const_cast(mBaseURI.get())); + nullptr, mURI); mURI = new PtrHolder(newURI.forget()); - mURIResolved = true; } return mURI; @@ -2707,11 +2706,10 @@ URLValue::URLValue(nsStringBuffer* aString, nsIURI* aBaseURI, nsIURI* aReferrer, MOZ_ASSERT(NS_IsMainThread()); } -URLValue::URLValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aBaseURI, - nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal) +URLValue::URLValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aReferrer, + nsIPrincipal* aOriginPrincipal) : URLValueData(do_AddRef(new PtrHolder(aURI)), aString, - do_AddRef(new PtrHolder(aBaseURI)), do_AddRef(new PtrHolder(aReferrer)), do_AddRef(new PtrHolder(aOriginPrincipal))) { @@ -2731,12 +2729,10 @@ css::URLValue::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const } css::ImageValue::ImageValue(nsIURI* aURI, nsStringBuffer* aString, - nsIURI* aBaseURI, nsIURI* aReferrer, - nsIPrincipal* aOriginPrincipal, + nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal, nsIDocument* aDocument) : URLValueData(do_AddRef(new PtrHolder(aURI)), aString, - do_AddRef(new PtrHolder(aBaseURI, false)), do_AddRef(new PtrHolder(aReferrer)), do_AddRef(new PtrHolder(aOriginPrincipal))) { diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h index 609ff781b565..2387abef152c 100644 --- a/layout/style/nsCSSValue.h +++ b/layout/style/nsCSSValue.h @@ -106,24 +106,20 @@ struct URLValueData // Construct with the actual URI. URLValueData(already_AddRefed> aURI, nsStringBuffer* aString, - already_AddRefed> aBaseURI, already_AddRefed> aReferrer, already_AddRefed> aOriginPrincipal); bool operator==(const URLValueData& aOther) const; - // Returns true iff we know for sure, by comparing the mBaseURI pointer, - // the specified url() value mString, and the mLocalURLFlag, that these - // two URLValueData objects represent the same computed url() value. - // - // Doesn't look at mReferrer or mOriginPrincipal. - // - // Safe to call from any thread. - bool DefinitelyEqualURIs(const URLValueData& aOther) const; + // URIEquals only compares URIs and principals (unlike operator==, which + // also compares the original strings). URIEquals also assumes that the + // mURI member of both URL objects is non-null. Do NOT call this method + // unless you're sure this is the case. + bool URIEquals(const URLValueData& aOther) const; - // Smae as DefinitelyEqualURIs but additionally compares the nsIPrincipal - // pointers of the two URLValueData objects. - bool DefinitelyEqualURIsAndPrincipal(const URLValueData& aOther) const; + // Pretty much like URIEquals, but allows comparing unresolved URIs (returning + // false in that case). + bool MaybeUnresolvedURIEquals(const URLValueData& aOther) const; nsIURI* GetURI() const; @@ -132,11 +128,11 @@ struct URLValueData bool GetLocalURLFlag() const { return mLocalURLFlag; } private: - // mURI stores the lazily resolved URI. This may be null if the URI is - // invalid, even once resolved. + // If mURIResolved is false, mURI stores the base URI. + // If mURIResolved is true, mURI stores the URI we resolve to; this may be + // null if the URI is invalid. mutable PtrHandle mURI; public: - PtrHandle mBaseURI; RefPtr mString; PtrHandle mReferrer; PtrHandle mOriginPrincipal; @@ -155,8 +151,8 @@ struct URLValue : public URLValueData // These two constructors are safe to call only on the main thread. URLValue(nsStringBuffer* aString, nsIURI* aBaseURI, nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal); - URLValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aBaseURI, - nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal); + URLValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aReferrer, + nsIPrincipal* aOriginPrincipal); // This constructor is safe to call from any thread. URLValue(nsStringBuffer* aString, @@ -186,9 +182,8 @@ struct ImageValue : public URLValueData // aString must not be null. // // This constructor is only safe to call from the main thread. - ImageValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aBaseURI, - nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal, - nsIDocument* aDocument); + ImageValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aReferrer, + nsIPrincipal* aOriginPrincipal, nsIDocument* aDocument); ImageValue(const ImageValue&) = delete; ImageValue& operator=(const ImageValue&) = delete; diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp index 7b4f5c3f94f2..d20a47e548a4 100644 --- a/layout/style/nsStyleStruct.cpp +++ b/layout/style/nsStyleStruct.cpp @@ -60,10 +60,10 @@ EqualURIs(nsIURI *aURI1, nsIURI *aURI2) } static bool -DefinitelyEqualURIsAndPrincipal(css::URLValue* aURI1, css::URLValue* aURI2) +MaybeUnresolvedURIEquals(css::URLValue *aURI1, css::URLValue *aURI2) { - return aURI1 == aURI2 || - (aURI1 && aURI2 && aURI1->DefinitelyEqualURIsAndPrincipal(*aURI2)); + return aURI1 == aURI2 || // handle null==null, and optimize + (aURI1 && aURI2 && aURI1->MaybeUnresolvedURIEquals(*aURI2)); } static @@ -3108,7 +3108,7 @@ nsStyleDisplay::CalcDifference(const nsStyleDisplay& aNewData) const { nsChangeHint hint = nsChangeHint(0); - if (!DefinitelyEqualURIsAndPrincipal(mBinding, aNewData.mBinding) + if (!MaybeUnresolvedURIEquals(mBinding, aNewData.mBinding) || mPosition != aNewData.mPosition || mDisplay != aNewData.mDisplay || mContain != aNewData.mContain