From 7afaed3c921aadc1f94bc7e50d1c5f5b0cf67367 Mon Sep 17 00:00:00 2001 From: Lee Salzman Date: Wed, 3 May 2017 20:45:39 -0400 Subject: [PATCH 01/15] Bug 1360862 - use FC_OUTLINE instead of FC_SCALABLE with Fontconfig to check if a font is scalable. r=jfkthame MozReview-Commit-ID: 1omWCJz5IK6 --- gfx/thebes/gfxFcPlatformFontList.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gfx/thebes/gfxFcPlatformFontList.cpp b/gfx/thebes/gfxFcPlatformFontList.cpp index dde4eb5a92e9..194685674ff3 100644 --- a/gfx/thebes/gfxFcPlatformFontList.cpp +++ b/gfx/thebes/gfxFcPlatformFontList.cpp @@ -1008,9 +1008,9 @@ gfxFontconfigFontFamily::AddFontPattern(FcPattern* aFontPattern) NS_ASSERTION(!mHasStyles, "font patterns must not be added to already enumerated families"); - FcBool scalable; - if (FcPatternGetBool(aFontPattern, FC_SCALABLE, 0, &scalable) != FcResultMatch || - !scalable) { + FcBool outline; + if (FcPatternGetBool(aFontPattern, FC_OUTLINE, 0, &outline) != FcResultMatch || + !outline) { mHasNonScalableFaces = true; } From 339c4768ca35cc45c4ab2f3820f552532cd23516 Mon Sep 17 00:00:00 2001 From: Yoshi Huang Date: Wed, 3 May 2017 10:08:14 +0800 Subject: [PATCH 02/15] Bug 1284579 - Part 1:revise NS_UsePrivateBrowsing to get PB from origin attributes. r=valentin --- netwerk/base/nsNetUtil.cpp | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/netwerk/base/nsNetUtil.cpp b/netwerk/base/nsNetUtil.cpp index 2263971a3cad..fbf6ae75637e 100644 --- a/netwerk/base/nsNetUtil.cpp +++ b/netwerk/base/nsNetUtil.cpp @@ -1565,16 +1565,10 @@ NS_LoadPersistentPropertiesFromURISpec(nsIPersistentProperties **outResult, bool NS_UsePrivateBrowsing(nsIChannel *channel) { - bool isPrivate = false; - nsCOMPtr pbChannel = do_QueryInterface(channel); - if (pbChannel && NS_SUCCEEDED(pbChannel->GetIsChannelPrivate(&isPrivate))) { - return isPrivate; - } - - // Some channels may not implement nsIPrivateBrowsingChannel - nsCOMPtr loadContext; - NS_QueryNotificationCallbacks(channel, loadContext); - return loadContext && loadContext->UsePrivateBrowsing(); + OriginAttributes attrs; + bool result = NS_GetOriginAttributes(channel, attrs); + NS_ENSURE_TRUE(result, result); + return attrs.mPrivateBrowsingId > 0; } bool @@ -1587,7 +1581,19 @@ NS_GetOriginAttributes(nsIChannel *aChannel, } loadInfo->GetOriginAttributes(&aAttributes); - aAttributes.SyncAttributesWithPrivateBrowsing(NS_UsePrivateBrowsing(aChannel)); + + bool isPrivate = false; + nsCOMPtr pbChannel = do_QueryInterface(aChannel); + if (pbChannel) { + nsresult rv = pbChannel->GetIsChannelPrivate(&isPrivate); + NS_ENSURE_SUCCESS(rv, false); + } else { + // Some channels may not implement nsIPrivateBrowsingChannel + nsCOMPtr loadContext; + NS_QueryNotificationCallbacks(aChannel, loadContext); + isPrivate = loadContext && loadContext->UsePrivateBrowsing(); + } + aAttributes.SyncAttributesWithPrivateBrowsing(isPrivate); return true; } From 7652396698a733f0adf5b498cc4f33ed1e8b229b Mon Sep 17 00:00:00 2001 From: Yoshi Huang Date: Wed, 3 May 2017 10:47:17 +0800 Subject: [PATCH 03/15] Bug 1284579 - Part 2: revise NS_ShouldCheckAppCache. r=valentin There's one redudant NS_ShouldCheckAppCache(nsIURI*, bool) is not used anymore. Also we remove the extra usePrivateBrowsing argument, since we can get this information from nsIPrincipal. --- docshell/base/nsDocShell.cpp | 4 +-- netwerk/base/nsNetUtil.cpp | 30 ++++----------------- netwerk/base/nsNetUtil.h | 6 ++--- netwerk/protocol/http/HttpChannelParent.cpp | 2 +- 4 files changed, 10 insertions(+), 32 deletions(-) diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 76f5762ae11f..2c6b715188a7 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -7571,7 +7571,7 @@ nsDocShell::OnRedirectStateChange(nsIChannel* aOldChannel, secMan->GetDocShellCodebasePrincipal(newURI, this, getter_AddRefs(principal)); appCacheChannel->SetChooseApplicationCache( - NS_ShouldCheckAppCache(principal, UsePrivateBrowsing())); + NS_ShouldCheckAppCache(principal)); } } } @@ -11160,7 +11160,7 @@ nsDocShell::DoURILoad(nsIURI* aURI, secMan->GetDocShellCodebasePrincipal(aURI, this, getter_AddRefs(principal)); appCacheChannel->SetChooseApplicationCache( - NS_ShouldCheckAppCache(principal, UsePrivateBrowsing())); + NS_ShouldCheckAppCache(principal)); } } } diff --git a/netwerk/base/nsNetUtil.cpp b/netwerk/base/nsNetUtil.cpp index fbf6ae75637e..7563c98e2bb0 100644 --- a/netwerk/base/nsNetUtil.cpp +++ b/netwerk/base/nsNetUtil.cpp @@ -1656,9 +1656,11 @@ NS_HasBeenCrossOrigin(nsIChannel* aChannel, bool aReport) } bool -NS_ShouldCheckAppCache(nsIURI *aURI, bool usePrivateBrowsing) +NS_ShouldCheckAppCache(nsIPrincipal *aPrincipal) { - if (usePrivateBrowsing) { + uint32_t privateBrowsingId = 0; + nsresult rv = aPrincipal->GetPrivateBrowsingId(&privateBrowsingId); + if (NS_SUCCEEDED(rv) && (privateBrowsingId > 0)) { return false; } @@ -1669,29 +1671,7 @@ NS_ShouldCheckAppCache(nsIURI *aURI, bool usePrivateBrowsing) } bool allowed; - nsresult rv = offlineService->OfflineAppAllowedForURI(aURI, - nullptr, - &allowed); - return NS_SUCCEEDED(rv) && allowed; -} - -bool -NS_ShouldCheckAppCache(nsIPrincipal *aPrincipal, bool usePrivateBrowsing) -{ - if (usePrivateBrowsing) { - return false; - } - - nsCOMPtr offlineService = - do_GetService("@mozilla.org/offlinecacheupdate-service;1"); - if (!offlineService) { - return false; - } - - bool allowed; - nsresult rv = offlineService->OfflineAppAllowed(aPrincipal, - nullptr, - &allowed); + rv = offlineService->OfflineAppAllowed(aPrincipal, nullptr, &allowed); return NS_SUCCEEDED(rv) && allowed; } diff --git a/netwerk/base/nsNetUtil.h b/netwerk/base/nsNetUtil.h index 2a1974282fea..f36e890c6051 100644 --- a/netwerk/base/nsNetUtil.h +++ b/netwerk/base/nsNetUtil.h @@ -659,11 +659,9 @@ bool NS_HasBeenCrossOrigin(nsIChannel* aChannel, bool aReport = false); "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" /** - * Determines whether appcache should be checked for a given URI. + * Determines whether appcache should be checked for a given principal. */ -bool NS_ShouldCheckAppCache(nsIURI *aURI, bool usePrivateBrowsing); - -bool NS_ShouldCheckAppCache(nsIPrincipal *aPrincipal, bool usePrivateBrowsing); +bool NS_ShouldCheckAppCache(nsIPrincipal *aPrincipal); /** * Wraps an nsIAuthPrompt so that it can be used as an nsIAuthPrompt2. This diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp index a4297e7123f4..41ea9be5d94d 100644 --- a/netwerk/protocol/http/HttpChannelParent.cpp +++ b/netwerk/protocol/http/HttpChannelParent.cpp @@ -590,7 +590,7 @@ HttpChannelParent::DoAsyncOpen( const URIParams& aURI, bool chooseAppCache = false; // This works because we've already called SetNotificationCallbacks and // done mPBOverride logic by this point. - chooseAppCache = NS_ShouldCheckAppCache(principal, NS_UsePrivateBrowsing(mChannel)); + chooseAppCache = NS_ShouldCheckAppCache(principal); appCacheChan->SetChooseApplicationCache(chooseAppCache); } From db11c81921e1f257b4fb5fff1c6e3ca44452519d Mon Sep 17 00:00:00 2001 From: Yoshi Huang Date: Wed, 3 May 2017 11:02:19 +0800 Subject: [PATCH 04/15] Bug 1284579 - Part 3: remove IsPrivate arg from nsCookieService. r=valentin Since we already passed origin attributes into these functions, we can remove the extra isPrivate argument. --- netwerk/cookie/CookieServiceParent.cpp | 15 +++++---------- netwerk/cookie/nsCookieService.cpp | 16 ++++------------ netwerk/cookie/nsCookieService.h | 4 ++-- 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/netwerk/cookie/CookieServiceParent.cpp b/netwerk/cookie/CookieServiceParent.cpp index b64128e3d76c..b33e340f1713 100644 --- a/netwerk/cookie/CookieServiceParent.cpp +++ b/netwerk/cookie/CookieServiceParent.cpp @@ -27,8 +27,7 @@ namespace { // Ignore failures from this function, as they only affect whether we do or // don't show a dialog box in private browsing mode if the user sets a pref. void -CreateDummyChannel(nsIURI* aHostURI, OriginAttributes& aAttrs, bool aIsPrivate, - nsIChannel** aChannel) +CreateDummyChannel(nsIURI* aHostURI, OriginAttributes& aAttrs, nsIChannel** aChannel) { nsCOMPtr principal = BasePrincipal::CreateCodebasePrincipal(aHostURI, aAttrs); @@ -53,7 +52,7 @@ CreateDummyChannel(nsIURI* aHostURI, OriginAttributes& aAttrs, bool aIsPrivate, return; } - pbChannel->SetPrivate(aIsPrivate); + pbChannel->SetPrivate(aAttrs.mPrivateBrowsingId > 0); dummyChannel.forget(aChannel); return; } @@ -101,9 +100,7 @@ CookieServiceParent::RecvGetCookieString(const URIParams& aHost, if (!hostURI) return IPC_FAIL_NO_REASON(this); - bool isPrivate = aAttrs.mPrivateBrowsingId > 0; - mCookieService->GetCookieStringInternal(hostURI, aIsForeign, false, aAttrs, - isPrivate, *aResult); + mCookieService->GetCookieStringInternal(hostURI, aIsForeign, false, aAttrs, *aResult); return IPC_OK(); } @@ -123,8 +120,6 @@ CookieServiceParent::RecvSetCookieString(const URIParams& aHost, if (!hostURI) return IPC_FAIL_NO_REASON(this); - bool isPrivate = aAttrs.mPrivateBrowsingId > 0; - // This is a gross hack. We've already computed everything we need to know // for whether to set this cookie or not, but we need to communicate all of // this information through to nsICookiePermission, which indirectly @@ -134,13 +129,13 @@ CookieServiceParent::RecvSetCookieString(const URIParams& aHost, // to use the channel to inspect it. nsCOMPtr dummyChannel; CreateDummyChannel(hostURI, const_cast(aAttrs), - isPrivate, getter_AddRefs(dummyChannel)); + getter_AddRefs(dummyChannel)); // NB: dummyChannel could be null if something failed in CreateDummyChannel. nsDependentCString cookieString(aCookieString, 0); mCookieService->SetCookieStringInternal(hostURI, aIsForeign, cookieString, aServerTime, false, aAttrs, - isPrivate, dummyChannel); + dummyChannel); return IPC_OK(); } diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp index 23e0cd6cadf4..7588097eaf6a 100644 --- a/netwerk/cookie/nsCookieService.cpp +++ b/netwerk/cookie/nsCookieService.cpp @@ -2011,11 +2011,8 @@ nsCookieService::GetCookieStringCommon(nsIURI *aHostURI, NS_GetOriginAttributes(aChannel, attrs); } - bool isPrivate = aChannel && NS_UsePrivateBrowsing(aChannel); - nsAutoCString result; - GetCookieStringInternal(aHostURI, isForeign, aHttpBound, attrs, - isPrivate, result); + GetCookieStringInternal(aHostURI, isForeign, aHttpBound, attrs, result); *aCookie = result.IsEmpty() ? nullptr : ToNewCString(result); return NS_OK; } @@ -2084,13 +2081,10 @@ nsCookieService::SetCookieStringCommon(nsIURI *aHostURI, NS_GetOriginAttributes(aChannel, attrs); } - bool isPrivate = aChannel && NS_UsePrivateBrowsing(aChannel); - nsDependentCString cookieString(aCookieHeader); nsDependentCString serverTime(aServerTime ? aServerTime : ""); SetCookieStringInternal(aHostURI, isForeign, cookieString, - serverTime, aFromHttp, attrs, - isPrivate, aChannel); + serverTime, aFromHttp, attrs, aChannel); return NS_OK; } @@ -2101,7 +2095,6 @@ nsCookieService::SetCookieStringInternal(nsIURI *aHostURI, const nsCString &aServerTime, bool aFromHttp, const OriginAttributes &aOriginAttrs, - bool aIsPrivate, nsIChannel *aChannel) { NS_ASSERTION(aHostURI, "null host!"); @@ -2112,7 +2105,7 @@ nsCookieService::SetCookieStringInternal(nsIURI *aHostURI, } AutoRestore savePrevDBState(mDBState); - mDBState = aIsPrivate ? mPrivateDBState : mDefaultDBState; + mDBState = (aOriginAttrs.mPrivateBrowsingId > 0) ? mPrivateDBState : mDefaultDBState; // get the base domain for the host URI. // e.g. for "www.bbc.co.uk", this would be "bbc.co.uk". @@ -3208,7 +3201,6 @@ nsCookieService::GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, const OriginAttributes& aOriginAttrs, - bool aIsPrivate, nsCString &aCookieString) { NS_ASSERTION(aHostURI, "null host!"); @@ -3219,7 +3211,7 @@ nsCookieService::GetCookieStringInternal(nsIURI *aHostURI, } AutoRestore savePrevDBState(mDBState); - mDBState = aIsPrivate ? mPrivateDBState : mDefaultDBState; + mDBState = (aOriginAttrs.mPrivateBrowsingId > 0) ? mPrivateDBState : mDefaultDBState; // get the base domain, host, and path from the URI. // e.g. for "www.bbc.co.uk", the base domain would be "bbc.co.uk". diff --git a/netwerk/cookie/nsCookieService.h b/netwerk/cookie/nsCookieService.h index f2c28499bcb2..b6d21c6d1ba5 100644 --- a/netwerk/cookie/nsCookieService.h +++ b/netwerk/cookie/nsCookieService.h @@ -294,9 +294,9 @@ class nsCookieService final : public nsICookieService nsresult GetBaseDomain(nsIURI *aHostURI, nsCString &aBaseDomain, bool &aRequireHostMatch); nsresult GetBaseDomainFromHost(const nsACString &aHost, nsCString &aBaseDomain); nsresult GetCookieStringCommon(nsIURI *aHostURI, nsIChannel *aChannel, bool aHttpBound, char** aCookie); - void GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, const OriginAttributes& aOriginAttrs, bool aIsPrivate, nsCString &aCookie); + void GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, const OriginAttributes& aOriginAttrs, nsCString &aCookie); nsresult SetCookieStringCommon(nsIURI *aHostURI, const char *aCookieHeader, const char *aServerTime, nsIChannel *aChannel, bool aFromHttp); - void SetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, nsDependentCString &aCookieHeader, const nsCString &aServerTime, bool aFromHttp, const OriginAttributes &aOriginAttrs, bool aIsPrivate, nsIChannel* aChannel); + void SetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, nsDependentCString &aCookieHeader, const nsCString &aServerTime, bool aFromHttp, const OriginAttributes &aOriginAttrs, nsIChannel* aChannel); bool SetCookieInternal(nsIURI *aHostURI, const nsCookieKey& aKey, bool aRequireHostMatch, CookieStatus aStatus, nsDependentCString &aCookieHeader, int64_t aServerTime, bool aFromHttp, nsIChannel* aChannel); void AddInternal(const nsCookieKey& aKey, nsCookie *aCookie, int64_t aCurrentTimeInUsec, nsIURI *aHostURI, const char *aCookieHeader, bool aFromHttp); void RemoveCookieFromList(const nsListIter &aIter, mozIStorageBindingParamsArray *aParamsArray = nullptr); From 82de893b0a75f8c582cadc361a29c53bd35c367a Mon Sep 17 00:00:00 2001 From: Timothy Nikkel Date: Wed, 3 May 2017 21:20:35 -0500 Subject: [PATCH 05/15] Bug 1360572. Invalidate the whole animated image when the composited frame becomes valid. r=aosmond We invalidate if FrameAnimator::UpdateState marks the composited frame as valid, but we fail to do so if FrameAnimator::RequestRefresh does so. The other place that sets the composited frame as valid is RasterImage::Decode. This call is actually redundant because the UpdateState call will do the same. Even though we will invalidate when the decode produces results we could still draw incorrectly if something else invalidates. In which case we would only draw the part of the image that was invalidated. But that should actually be impossible as explained in the comment. --- image/FrameAnimator.cpp | 1 + image/RasterImage.cpp | 25 ++++++++++++++----------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/image/FrameAnimator.cpp b/image/FrameAnimator.cpp index 4602782af990..f96952ee1ac6 100644 --- a/image/FrameAnimator.cpp +++ b/image/FrameAnimator.cpp @@ -425,6 +425,7 @@ FrameAnimator::RequestRefresh(AnimationState& aState, // Advanced to the correct frame, the composited frame is now valid to be drawn. if (*currentFrameEndTime > aTime) { aState.mCompositedFrameInvalid = false; + ret.mDirtyRect = IntRect(IntPoint(0,0), mSize); } MOZ_ASSERT(!aState.mIsCurrentlyDecoded || !aState.mCompositedFrameInvalid); diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp index f5d62d6259d1..f89573b1d300 100644 --- a/image/RasterImage.cpp +++ b/image/RasterImage.cpp @@ -1262,17 +1262,20 @@ RasterImage::Decode(const IntSize& aSize, mSourceBuffer, mSize, decoderFlags, surfaceFlags); // We may not be able to send an invalidation right here because of async - // notifications but that's not a problem because the first frame - // invalidation (when it comes) will invalidate for us. So we can ignore - // the return value of UpdateState. This also handles the invalidation - // from setting the composited frame as valid below. - mAnimationState->UpdateState(mAnimationFinished, this, mSize); - // If the animation is finished we can draw right away because we just draw - // the final frame all the time from now on. See comment in - // AnimationState::UpdateState. - if (mAnimationFinished) { - mAnimationState->SetCompositedFrameInvalid(false); - } + // notifications but that shouldn't be a problem because we shouldn't be + // getting a non-empty rect back from UpdateState. This is because UpdateState + // will only return a non-empty rect if we are currently decoded, or the + // animation is finished. We can't be decoded because we are creating a decoder + // here. If the animation is finished then the composited frame would have + // been valid when the animation finished, and it's not possible to mark + // the composited frame as invalid when the animation is finished. So + // the composited frame can't change from invalid to valid in this UpdateState + // call, and hence no rect can be returned. +#ifdef DEBUG + gfx::IntRect rect = +#endif + mAnimationState->UpdateState(mAnimationFinished, this, mSize); + MOZ_ASSERT(rect.IsEmpty()); } else { task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this), mSourceBuffer, mSize, aSize, From 14b9ff6984a512987fb59ab68f0f99c77d14dc1b Mon Sep 17 00:00:00 2001 From: Eric Rahm Date: Wed, 3 May 2017 19:22:39 -0700 Subject: [PATCH 06/15] Bug 935809 - Part 0: Include assertion header in DoublyLinkedList.h. r=waldo MozReview-Commit-ID: 4G37uslYlOb --- mfbt/DoublyLinkedList.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mfbt/DoublyLinkedList.h b/mfbt/DoublyLinkedList.h index 3d4cc3c4da84..d5db36131009 100644 --- a/mfbt/DoublyLinkedList.h +++ b/mfbt/DoublyLinkedList.h @@ -12,6 +12,8 @@ #include #include +#include "mozilla/Assertions.h" + /** * Where mozilla::LinkedList strives for ease of use above all other * considerations, mozilla::DoublyLinkedList strives for flexibility. The From 7900dc0fabdb628d84d55d0ad641dd5a8ec371eb Mon Sep 17 00:00:00 2001 From: Eric Rahm Date: Wed, 3 May 2017 19:22:45 -0700 Subject: [PATCH 07/15] Bug 935809 - Part 1: Convert breakpoint lists to DoublyLinkedList. r=jimb MozReview-Commit-ID: J4jdqLOksND --- js/src/vm/Debugger.cpp | 39 +++++++++------------------ js/src/vm/Debugger.h | 60 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 35 deletions(-) diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index 6b2aa8739b11..903d45cd2f68 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -524,7 +524,6 @@ RequireGlobalObject(JSContext* cx, HandleValue dbgobj, HandleObject referent) BreakpointSite::BreakpointSite(Type type) : type_(type), enabledCount(0) { - JS_INIT_CLIST(&breakpoints); } void @@ -547,21 +546,22 @@ BreakpointSite::dec(FreeOp* fop) bool BreakpointSite::isEmpty() const { - return JS_CLIST_IS_EMPTY(&breakpoints); + return breakpoints.isEmpty(); } Breakpoint* BreakpointSite::firstBreakpoint() const { - if (JS_CLIST_IS_EMPTY(&breakpoints)) + if (isEmpty()) return nullptr; - return Breakpoint::fromSiteLinks(JS_NEXT_LINK(&breakpoints)); + return &(*breakpoints.begin()); } bool -BreakpointSite::hasBreakpoint(Breakpoint* bp) +BreakpointSite::hasBreakpoint(Breakpoint* toFind) { - for (Breakpoint* p = firstBreakpoint(); p; p = p->nextInSite()) + const BreakpointList::Iterator bp(toFind); + for (auto p = breakpoints.begin(); p; p++) if (p == bp) return true; return false; @@ -571,20 +571,8 @@ Breakpoint::Breakpoint(Debugger* debugger, BreakpointSite* site, JSObject* handl : debugger(debugger), site(site), handler(handler) { MOZ_ASSERT(handler->compartment() == debugger->object->compartment()); - JS_APPEND_LINK(&debuggerLinks, &debugger->breakpoints); - JS_APPEND_LINK(&siteLinks, &site->breakpoints); -} - -Breakpoint* -Breakpoint::fromDebuggerLinks(JSCList* links) -{ - return (Breakpoint*) ((unsigned char*) links - offsetof(Breakpoint, debuggerLinks)); -} - -Breakpoint* -Breakpoint::fromSiteLinks(JSCList* links) -{ - return (Breakpoint*) ((unsigned char*) links - offsetof(Breakpoint, siteLinks)); + debugger->breakpoints.pushBack(this); + site->breakpoints.pushBack(this); } void @@ -592,8 +580,8 @@ Breakpoint::destroy(FreeOp* fop) { if (debugger->enabled) site->dec(fop); - JS_REMOVE_LINK(&debuggerLinks); - JS_REMOVE_LINK(&siteLinks); + debugger->breakpoints.remove(this); + site->breakpoints.remove(this); site->destroyIfEmpty(fop); fop->delete_(this); } @@ -601,15 +589,13 @@ Breakpoint::destroy(FreeOp* fop) Breakpoint* Breakpoint::nextInDebugger() { - JSCList* link = JS_NEXT_LINK(&debuggerLinks); - return (link == &debugger->breakpoints) ? nullptr : fromDebuggerLinks(link); + return debuggerLink.mNext; } Breakpoint* Breakpoint::nextInSite() { - JSCList* link = JS_NEXT_LINK(&siteLinks); - return (link == &site->breakpoints) ? nullptr : fromSiteLinks(link); + return siteLink.mNext; } JSBreakpointSite::JSBreakpointSite(JSScript* script, jsbytecode* pc) @@ -684,7 +670,6 @@ Debugger::Debugger(JSContext* cx, NativeObject* dbg) { assertSameCompartment(cx, dbg); - JS_INIT_CLIST(&breakpoints); JS_INIT_CLIST(&onNewGlobalObjectWatchersLink); #ifdef JS_TRACE_LOGGING diff --git a/js/src/vm/Debugger.h b/js/src/vm/Debugger.h index e3f9d82850a4..5c9362c792c0 100644 --- a/js/src/vm/Debugger.h +++ b/js/src/vm/Debugger.h @@ -7,6 +7,7 @@ #ifndef vm_Debugger_h #define vm_Debugger_h +#include "mozilla/DoublyLinkedList.h" #include "mozilla/GuardObjects.h" #include "mozilla/LinkedList.h" #include "mozilla/Range.h" @@ -386,7 +387,27 @@ class Debugger : private mozilla::LinkedListElement // Whether to enable code coverage on the Debuggee. bool collectCoverageInfo; - JSCList breakpoints; /* Circular list of all js::Breakpoints in this debugger */ + template + struct DebuggerSiblingAccess { + static T* GetNext(T* elm) { + return elm->debuggerLink.mNext; + } + static void SetNext(T* elm, T* next) { + elm->debuggerLink.mNext = next; + } + static T* GetPrev(T* elm) { + return elm->debuggerLink.mPrev; + } + static void SetPrev(T* elm, T* prev) { + elm->debuggerLink.mPrev = prev; + } + }; + + // List of all js::Breakpoints in this debugger. + using BreakpointList = + mozilla::DoublyLinkedList>; + BreakpointList breakpoints; // The set of GC numbers for which one or more of this Debugger's observed // debuggees participated in. @@ -1563,7 +1584,27 @@ class BreakpointSite { private: Type type_; - JSCList breakpoints; /* cyclic list of all js::Breakpoints at this instruction */ + template + struct SiteSiblingAccess { + static T* GetNext(T* elm) { + return elm->siteLink.mNext; + } + static void SetNext(T* elm, T* next) { + elm->siteLink.mNext = next; + } + static T* GetPrev(T* elm) { + return elm->siteLink.mPrev; + } + static void SetPrev(T* elm, T* prev) { + elm->siteLink.mPrev = prev; + } + }; + + // List of all js::Breakpoints at this instruction. + using BreakpointList = + mozilla::DoublyLinkedList>; + BreakpointList breakpoints; size_t enabledCount; /* number of breakpoints in the list that are enabled */ protected: @@ -1607,6 +1648,7 @@ class BreakpointSite { class Breakpoint { friend struct ::JSCompartment; friend class Debugger; + friend class BreakpointSite; public: Debugger * const debugger; @@ -1614,12 +1656,14 @@ class Breakpoint { private: /* |handler| is marked unconditionally during minor GC. */ js::PreBarrieredObject handler; - JSCList debuggerLinks; - JSCList siteLinks; + + /** + * Link elements for each list this breakpoint can be in. + */ + mozilla::DoublyLinkedListElement debuggerLink; + mozilla::DoublyLinkedListElement siteLink; public: - static Breakpoint* fromDebuggerLinks(JSCList* links); - static Breakpoint* fromSiteLinks(JSCList* links); Breakpoint(Debugger* debugger, BreakpointSite* site, JSObject* handler); void destroy(FreeOp* fop); Breakpoint* nextInDebugger(); @@ -1697,9 +1741,9 @@ Breakpoint::asWasm() Breakpoint* Debugger::firstBreakpoint() const { - if (JS_CLIST_IS_EMPTY(&breakpoints)) + if (breakpoints.isEmpty()) return nullptr; - return Breakpoint::fromDebuggerLinks(JS_NEXT_LINK(&breakpoints)); + return &(*breakpoints.begin()); } /* static */ Debugger* From e5680a660e5004c69f197d176fa80dc3f54b4e5a Mon Sep 17 00:00:00 2001 From: Eric Rahm Date: Wed, 3 May 2017 19:22:51 -0700 Subject: [PATCH 08/15] Bug 935809 - Part 2: Convert onNewGlobalUpdateWatchers to DoublyLinkedList. r=jimb MozReview-Commit-ID: 75SY0ab5gxn --- js/src/vm/Debugger.cpp | 42 +++++++++++++----------------------------- js/src/vm/Debugger.h | 19 +++++-------------- js/src/vm/Runtime.cpp | 1 - js/src/vm/Runtime.h | 30 +++++++++++++++++++++++++----- 4 files changed, 43 insertions(+), 49 deletions(-) diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index 903d45cd2f68..a71d00e46168 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -670,8 +670,6 @@ Debugger::Debugger(JSContext* cx, NativeObject* dbg) { assertSameCompartment(cx, dbg); - JS_INIT_CLIST(&onNewGlobalObjectWatchersLink); - #ifdef JS_TRACE_LOGGING TraceLoggerThread* logger = TraceLoggerForCurrentThread(cx); if (logger) { @@ -690,15 +688,15 @@ Debugger::~Debugger() allocationsLog.clear(); /* - * Since the inactive state for this link is a singleton cycle, it's always - * safe to apply JS_REMOVE_LINK to it, regardless of whether we're in the list or not. - * * We don't have to worry about locking here since Debugger is not * background finalized. */ - JS_REMOVE_LINK(&onNewGlobalObjectWatchersLink); - JSContext* cx = TlsContext.get(); + if (onNewGlobalObjectWatchersLink.mPrev || + onNewGlobalObjectWatchersLink.mNext || + cx->runtime()->onNewGlobalObjectWatchers().begin() == JSRuntime::WatchersList::Iterator(this)) + cx->runtime()->onNewGlobalObjectWatchers().remove(this); + cx->runtime()->endSingleThreadedExecution(cx); } @@ -2163,7 +2161,7 @@ Debugger::fireNewGlobalObject(JSContext* cx, Handle global, Mutab void Debugger::slowPathOnNewGlobalObject(JSContext* cx, Handle global) { - MOZ_ASSERT(!JS_CLIST_IS_EMPTY(&cx->runtime()->onNewGlobalObjectWatchers())); + MOZ_ASSERT(!cx->runtime()->onNewGlobalObjectWatchers().isEmpty()); if (global->compartment()->creationOptions().invisibleToDebugger()) return; @@ -2173,13 +2171,9 @@ Debugger::slowPathOnNewGlobalObject(JSContext* cx, Handle global) * can be mutated while we're walking it. */ AutoObjectVector watchers(cx); - for (JSCList* link = JS_LIST_HEAD(&cx->runtime()->onNewGlobalObjectWatchers()); - link != &cx->runtime()->onNewGlobalObjectWatchers(); - link = JS_NEXT_LINK(link)) - { - Debugger* dbg = fromOnNewGlobalObjectWatchersLink(link); - MOZ_ASSERT(dbg->observesNewGlobalObject()); - JSObject* obj = dbg->object; + for (auto& dbg : cx->runtime()->onNewGlobalObjectWatchers()) { + MOZ_ASSERT(dbg.observesNewGlobalObject()); + JSObject* obj = dbg.object; JS::ExposeObjectToActiveJS(obj); if (!watchers.append(obj)) { if (cx->isExceptionPending()) @@ -3360,14 +3354,9 @@ Debugger::setEnabled(JSContext* cx, unsigned argc, Value* vp) */ if (dbg->getHook(OnNewGlobalObject)) { if (!wasEnabled) { - /* If we were not enabled, the link should be a singleton list. */ - MOZ_ASSERT(JS_CLIST_IS_EMPTY(&dbg->onNewGlobalObjectWatchersLink)); - JS_APPEND_LINK(&dbg->onNewGlobalObjectWatchersLink, - &cx->runtime()->onNewGlobalObjectWatchers()); + cx->runtime()->onNewGlobalObjectWatchers().pushBack(dbg); } else { - /* If we were enabled, the link should be inserted in the list. */ - MOZ_ASSERT(!JS_CLIST_IS_EMPTY(&dbg->onNewGlobalObjectWatchersLink)); - JS_REMOVE_AND_INIT_LINK(&dbg->onNewGlobalObjectWatchersLink); + cx->runtime()->onNewGlobalObjectWatchers().remove(dbg); } } @@ -3527,14 +3516,9 @@ Debugger::setOnNewGlobalObject(JSContext* cx, unsigned argc, Value* vp) if (dbg->enabled) { JSObject* newHook = dbg->getHook(OnNewGlobalObject); if (!oldHook && newHook) { - /* If we didn't have a hook, the link should be a singleton list. */ - MOZ_ASSERT(JS_CLIST_IS_EMPTY(&dbg->onNewGlobalObjectWatchersLink)); - JS_APPEND_LINK(&dbg->onNewGlobalObjectWatchersLink, - &cx->runtime()->onNewGlobalObjectWatchers()); + cx->runtime()->onNewGlobalObjectWatchers().pushBack(dbg); } else if (oldHook && !newHook) { - /* If we did have a hook, the link should be inserted in the list. */ - MOZ_ASSERT(!JS_CLIST_IS_EMPTY(&dbg->onNewGlobalObjectWatchersLink)); - JS_REMOVE_AND_INIT_LINK(&dbg->onNewGlobalObjectWatchersLink); + cx->runtime()->onNewGlobalObjectWatchers().remove(dbg); } } diff --git a/js/src/vm/Debugger.h b/js/src/vm/Debugger.h index 5c9362c792c0..7a21af783151 100644 --- a/js/src/vm/Debugger.h +++ b/js/src/vm/Debugger.h @@ -14,7 +14,6 @@ #include "mozilla/TimeStamp.h" #include "mozilla/Vector.h" -#include "jsclist.h" #include "jscntxt.h" #include "jscompartment.h" #include "jsweakmap.h" @@ -260,6 +259,7 @@ class Debugger : private mozilla::LinkedListElement { friend class Breakpoint; friend class DebuggerMemory; + friend struct JSRuntime::GlobalObjectWatchersSiblingAccess; friend class SavedStacks; friend class ScriptedOnStepHandler; friend class ScriptedOnPopHandler; @@ -467,11 +467,10 @@ class Debugger : private mozilla::LinkedListElement /* * If this Debugger is enabled, and has a onNewGlobalObject handler, then - * this link is inserted into the circular list headed by - * JSRuntime::onNewGlobalObjectWatchers. Otherwise, this is set to a - * singleton cycle. + * this link is inserted into the list headed by + * JSRuntime::onNewGlobalObjectWatchers. */ - JSCList onNewGlobalObjectWatchersLink; + mozilla::DoublyLinkedListElement onNewGlobalObjectWatchersLink; /* * Map from stack frames that are currently on the stack to Debugger.Frame @@ -811,8 +810,6 @@ class Debugger : private mozilla::LinkedListElement inline Breakpoint* firstBreakpoint() const; - static inline Debugger* fromOnNewGlobalObjectWatchersLink(JSCList* link); - static MOZ_MUST_USE bool replaceFrameGuts(JSContext* cx, AbstractFramePtr from, AbstractFramePtr to, ScriptFrameIter& iter); @@ -1746,12 +1743,6 @@ Debugger::firstBreakpoint() const return &(*breakpoints.begin()); } -/* static */ Debugger* -Debugger::fromOnNewGlobalObjectWatchersLink(JSCList* link) { - char* p = reinterpret_cast(link); - return reinterpret_cast(p - offsetof(Debugger, onNewGlobalObjectWatchersLink)); -} - const js::GCPtrNativeObject& Debugger::toJSObject() const { @@ -1810,7 +1801,7 @@ Debugger::onNewGlobalObject(JSContext* cx, Handle global) #ifdef DEBUG global->compartment()->firedOnNewGlobalObject = true; #endif - if (!JS_CLIST_IS_EMPTY(&cx->runtime()->onNewGlobalObjectWatchers())) + if (!cx->runtime()->onNewGlobalObjectWatchers().isEmpty()) Debugger::slowPathOnNewGlobalObject(cx, global); } diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index a194f21b2392..9b7337be6adf 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -177,7 +177,6 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime) liveRuntimesCount++; /* Initialize infallibly first, so we can goto bad and JS_DestroyRuntime. */ - JS_INIT_CLIST(&onNewGlobalObjectWatchers()); PodZero(&asmJSCacheOps); lcovOutput().init(); diff --git a/js/src/vm/Runtime.h b/js/src/vm/Runtime.h index 91c606a1fa69..539a4e90d70a 100644 --- a/js/src/vm/Runtime.h +++ b/js/src/vm/Runtime.h @@ -9,6 +9,7 @@ #include "mozilla/Atomics.h" #include "mozilla/Attributes.h" +#include "mozilla/DoublyLinkedList.h" #include "mozilla/LinkedList.h" #include "mozilla/MemoryReporting.h" #include "mozilla/PodOperations.h" @@ -19,7 +20,6 @@ #include #include "jsatom.h" -#include "jsclist.h" #include "jsscript.h" #ifdef XP_DARWIN @@ -560,14 +560,34 @@ struct JSRuntime : public js::MallocProvider weakCaches().insertBack(cachep); } + template + struct GlobalObjectWatchersSiblingAccess { + static T* GetNext(T* elm) { + return elm->onNewGlobalObjectWatchersLink.mNext; + } + static void SetNext(T* elm, T* next) { + elm->onNewGlobalObjectWatchersLink.mNext = next; + } + static T* GetPrev(T* elm) { + return elm->onNewGlobalObjectWatchersLink.mPrev; + } + static void SetPrev(T* elm, T* prev) { + elm->onNewGlobalObjectWatchersLink.mPrev = prev; + } + }; + + using WatchersList = + mozilla::DoublyLinkedList>; private: /* - * Head of circular list of all enabled Debuggers that have - * onNewGlobalObject handler methods established. + * List of all enabled Debuggers that have onNewGlobalObject handler + * methods established. */ - js::ActiveThreadData onNewGlobalObjectWatchers_; + js::ActiveThreadData onNewGlobalObjectWatchers_; + public: - JSCList& onNewGlobalObjectWatchers() { return onNewGlobalObjectWatchers_.ref(); } + WatchersList& onNewGlobalObjectWatchers() { return onNewGlobalObjectWatchers_.ref(); } private: /* From edf884fdd11c39e203ad276599c465686bc2b2b4 Mon Sep 17 00:00:00 2001 From: Eric Rahm Date: Wed, 3 May 2017 19:22:57 -0700 Subject: [PATCH 09/15] Bug 935809 - Part 3: Remove JSCList. r=jimb MozReview-Commit-ID: Du6i1FlJ3Uu --- js/src/jsclist.h | 107 ----------------------------------------------- js/src/moz.build | 1 - 2 files changed, 108 deletions(-) delete mode 100644 js/src/jsclist.h diff --git a/js/src/jsclist.h b/js/src/jsclist.h deleted file mode 100644 index b8455152c63c..000000000000 --- a/js/src/jsclist.h +++ /dev/null @@ -1,107 +0,0 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- - * vim: set ts=8 sts=4 et sw=4 tw=99: - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef jsclist_h -#define jsclist_h - -#include "jstypes.h" - -/* -** Circular linked list -*/ -typedef struct JSCListStr { - struct JSCListStr* next; - struct JSCListStr* prev; -} JSCList; - -/* -** Insert element "_e" into the list, before "_l". -*/ -#define JS_INSERT_BEFORE(_e,_l) \ - JS_BEGIN_MACRO \ - (_e)->next = (_l); \ - (_e)->prev = (_l)->prev; \ - (_l)->prev->next = (_e); \ - (_l)->prev = (_e); \ - JS_END_MACRO - -/* -** Insert element "_e" into the list, after "_l". -*/ -#define JS_INSERT_AFTER(_e,_l) \ - JS_BEGIN_MACRO \ - (_e)->next = (_l)->next; \ - (_e)->prev = (_l); \ - (_l)->next->prev = (_e); \ - (_l)->next = (_e); \ - JS_END_MACRO - -/* -** Return the element following element "_e" -*/ -#define JS_NEXT_LINK(_e) \ - ((_e)->next) -/* -** Return the element preceding element "_e" -*/ -#define JS_PREV_LINK(_e) \ - ((_e)->prev) - -/* -** Append an element "_e" to the end of the list "_l" -*/ -#define JS_APPEND_LINK(_e,_l) JS_INSERT_BEFORE(_e,_l) - -/* -** Insert an element "_e" at the head of the list "_l" -*/ -#define JS_INSERT_LINK(_e,_l) JS_INSERT_AFTER(_e,_l) - -/* Return the head/tail of the list */ -#define JS_LIST_HEAD(_l) (_l)->next -#define JS_LIST_TAIL(_l) (_l)->prev - -/* -** Remove the element "_e" from it's circular list. -*/ -#define JS_REMOVE_LINK(_e) \ - JS_BEGIN_MACRO \ - (_e)->prev->next = (_e)->next; \ - (_e)->next->prev = (_e)->prev; \ - JS_END_MACRO - -/* -** Remove the element "_e" from it's circular list. Also initializes the -** linkage. -*/ -#define JS_REMOVE_AND_INIT_LINK(_e) \ - JS_BEGIN_MACRO \ - (_e)->prev->next = (_e)->next; \ - (_e)->next->prev = (_e)->prev; \ - (_e)->next = (_e); \ - (_e)->prev = (_e); \ - JS_END_MACRO - -/* -** Return non-zero if the given circular list "_l" is empty, zero if the -** circular list is not empty -*/ -#define JS_CLIST_IS_EMPTY(_l) \ - bool((_l)->next == (_l)) - -/* -** Initialize a circular list -*/ -#define JS_INIT_CLIST(_l) \ - JS_BEGIN_MACRO \ - (_l)->next = (_l); \ - (_l)->prev = (_l); \ - JS_END_MACRO - -#define JS_INIT_STATIC_CLIST(_l) \ - {(_l), (_l)} - -#endif /* jsclist_h */ diff --git a/js/src/moz.build b/js/src/moz.build index 99d01c3f0cd1..5b34de0f2382 100644 --- a/js/src/moz.build +++ b/js/src/moz.build @@ -81,7 +81,6 @@ EXPORTS += [ 'jsalloc.h', 'jsapi.h', 'jsbytecode.h', - 'jsclist.h', 'jscpucfg.h', 'jsfriendapi.h', 'jsprf.h', From e77423bec54de97278e790ede7dea72aeb6034aa Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Mon, 17 Apr 2017 21:41:18 -0700 Subject: [PATCH 10/15] Bug 1353060: Use the correct layer manager for frameloaders in s. r=kats,mattwoodrow Currently, we only correctly support remote layer trees for frameloaders that use the same layer manager as their document. Since we need to be able to host remote content in popup widgets for remote WebExtensions, we need to tie the frameloaders to the layer manager of their host element, rather than the root layer manager for the document. MozReview-Commit-ID: 4RCsamFBiQw --HG-- extra : rebase_source : 86bca4ae1c012ff1bb84a9ad796be311cfe580f6 extra : histedit_source : 19577d69430adc8cb38c195f13db2c6de6605c4c --- dom/base/nsContentUtils.cpp | 28 ++++++++++++++++++++++++++++ dom/base/nsContentUtils.h | 24 ++++++++++++++++++++++++ dom/base/nsFrameLoader.cpp | 9 ++++++--- dom/ipc/TabParent.cpp | 8 ++++---- layout/ipc/RenderFrameParent.cpp | 31 +++++++++++++++++++++++++------ layout/ipc/RenderFrameParent.h | 3 +++ 6 files changed, 90 insertions(+), 13 deletions(-) diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index 7a7fa9fd61e6..b0ca39654732 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -6769,6 +6769,34 @@ nsContentUtils::WidgetForDocument(const nsIDocument* aDoc) return nullptr; } +nsIWidget* +nsContentUtils::WidgetForContent(const nsIContent* aContent) +{ + nsIFrame* frame = aContent->GetPrimaryFrame(); + if (frame) { + frame = nsLayoutUtils::GetDisplayRootFrame(frame); + + nsView* view = frame->GetView(); + if (view) { + return view->GetWidget(); + } + } + + return nullptr; +} + +already_AddRefed +nsContentUtils::LayerManagerForContent(const nsIContent *aContent) +{ + nsIWidget* widget = nsContentUtils::WidgetForContent(aContent); + if (widget) { + RefPtr manager = widget->GetLayerManager(); + return manager.forget(); + } + + return nullptr; +} + static already_AddRefed LayerManagerForDocumentInternal(const nsIDocument *aDoc, bool aRequirePersistent) { diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index a520d89e6f57..736b43a750aa 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -2026,15 +2026,29 @@ public: * Returns the widget for this document if there is one. Looks at all ancestor * documents to try to find a widget, so for example this can still find a * widget for documents in display:none frames that have no presentation. + * + * You should probably use WidgetForContent() instead of this, unless you have + * a good reason to do otherwise. */ static nsIWidget* WidgetForDocument(const nsIDocument* aDoc); + /** + * Returns the appropriate widget for this element, if there is one. Unlike + * WidgetForDocument(), this returns the correct widget for content in popups. + * + * You should probably use this instead of WidgetForDocument(). + */ + static nsIWidget* WidgetForContent(const nsIContent* aContent); + /** * Returns a layer manager to use for the given document. Basically we * look up the document hierarchy for the first document which has * a presentation with an associated widget, and use that widget's * layer manager. * + * You should probably use LayerManagerForContent() instead of this, unless + * you have a good reason to do otherwise. + * * @param aDoc the document for which to return a layer manager. * @param aAllowRetaining an outparam that states whether the returned * layer manager should be used for retained layers @@ -2042,6 +2056,16 @@ public: static already_AddRefed LayerManagerForDocument(const nsIDocument *aDoc); + /** + * Returns a layer manager to use for the given content. Unlike + * LayerManagerForDocument(), this returns the correct layer manager for + * content in popups. + * + * You should probably use this instead of LayerManagerForDocument(). + */ + static already_AddRefed + LayerManagerForContent(const nsIContent *aContent); + /** * Returns a layer manager to use for the given document. Basically we * look up the document hierarchy for the first document which has diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp index a9180d695540..a1b614b61ca6 100644 --- a/dom/base/nsFrameLoader.cpp +++ b/dom/base/nsFrameLoader.cpp @@ -1249,9 +1249,12 @@ nsFrameLoader::ShowRemoteFrame(const ScreenIntSize& size, return false; } - RefPtr layerManager = - nsContentUtils::LayerManagerForDocument(mOwnerContent->GetComposedDoc()); - if (!layerManager) { + RenderFrameParent* rfp = GetCurrentRenderFrame(); + if (!rfp) { + return false; + } + + if (!rfp->AttachLayerManager()) { // This is just not going to work. return false; } diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp index 2e9ff5f76f2b..670e7839ed04 100644 --- a/dom/ipc/TabParent.cpp +++ b/dom/ipc/TabParent.cpp @@ -2293,7 +2293,7 @@ TabParent::RecvGetDPI(float* aValue) { TryCacheDPIAndScale(); - MOZ_ASSERT(mDPI > 0, + MOZ_ASSERT(mDPI > 0 || mFrameElement, "Must not ask for DPI before OwnerElement is received!"); *aValue = mDPI; return IPC_OK(); @@ -2304,7 +2304,7 @@ TabParent::RecvGetDefaultScale(double* aValue) { TryCacheDPIAndScale(); - MOZ_ASSERT(mDefaultScale.scale > 0, + MOZ_ASSERT(mDefaultScale.scale > 0 || mFrameElement, "Must not ask for scale before OwnerElement is received!"); *aValue = mDefaultScale.scale; return IPC_OK(); @@ -2315,7 +2315,7 @@ TabParent::RecvGetWidgetRounding(int32_t* aValue) { TryCacheDPIAndScale(); - MOZ_ASSERT(mRounding > 0, + MOZ_ASSERT(mRounding > 0 || mFrameElement, "Must not ask for rounding before OwnerElement is received!"); *aValue = mRounding; return IPC_OK(); @@ -2581,7 +2581,7 @@ TabParent::GetWidget() const if (!mFrameElement) { return nullptr; } - nsCOMPtr widget = nsContentUtils::WidgetForDocument(mFrameElement->OwnerDoc()); + nsCOMPtr widget = nsContentUtils::WidgetForContent(mFrameElement); return widget.forget(); } diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp index f33f32f04647..ea8b776fbfda 100644 --- a/layout/ipc/RenderFrameParent.cpp +++ b/layout/ipc/RenderFrameParent.cpp @@ -146,6 +146,7 @@ void RenderFrameParent::Destroy() { mFrameLoaderDestroyed = true; + mLayerManager = nullptr; } already_AddRefed @@ -206,18 +207,35 @@ RenderFrameParent::BuildLayer(nsDisplayListBuilder* aBuilder, return layer.forget(); } +LayerManager* +RenderFrameParent::AttachLayerManager() +{ + RefPtr lm; + + if (mFrameLoader) { + nsIContent* content = mFrameLoader->GetOwnerContent(); + if (content) { + lm = nsContentUtils::LayerManagerForContent(content); + } + } + + // Perhaps the document containing this frame currently has no presentation? + if (lm && lm->GetCompositorBridgeChild() && lm != mLayerManager) { + mLayersConnected = lm->GetCompositorBridgeChild()->SendAdoptChild(mLayersId); + FrameLayerBuilder::InvalidateAllLayers(lm); + } + + mLayerManager = lm.forget(); + return mLayerManager; +} + void RenderFrameParent::OwnerContentChanged(nsIContent* aContent) { MOZ_ASSERT(!mFrameLoader || mFrameLoader->GetOwnerContent() == aContent, "Don't build new map if owner is same!"); - RefPtr lm = mFrameLoader ? GetFrom(mFrameLoader) : nullptr; - // Perhaps the document containing this frame currently has no presentation? - if (lm && lm->GetCompositorBridgeChild()) { - mLayersConnected = lm->GetCompositorBridgeChild()->SendAdoptChild(mLayersId); - FrameLayerBuilder::InvalidateAllLayers(lm); - } + Unused << AttachLayerManager(); } void @@ -232,6 +250,7 @@ RenderFrameParent::ActorDestroy(ActorDestroyReason why) } mFrameLoader = nullptr; + mLayerManager = nullptr; } mozilla::ipc::IPCResult diff --git a/layout/ipc/RenderFrameParent.h b/layout/ipc/RenderFrameParent.h index 693849c80921..b1b40b1190ca 100644 --- a/layout/ipc/RenderFrameParent.h +++ b/layout/ipc/RenderFrameParent.h @@ -90,6 +90,8 @@ public: void EnsureLayersConnected(CompositorOptions* aCompositorOptions); + LayerManager* AttachLayerManager(); + protected: void ActorDestroy(ActorDestroyReason why) override; @@ -114,6 +116,7 @@ private: RefPtr mFrameLoader; RefPtr mContainer; + RefPtr mLayerManager; // True after Destroy() has been called, which is triggered // originally by nsFrameLoader::Destroy(). After this point, we can From cfa6b13d1dd40d48329a0d1108d3098f1743f768 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Mon, 17 Apr 2017 13:45:08 -0700 Subject: [PATCH 11/15] Bug 1353060: Disable remote frameloaders in small popup widgets. r=kats Since we now want to support APZ for remote frameloaders in popups, but do not want to needlessly enable it in simple popup widgets which should never host remote content, we need to prevent remote content from being loaded into those popups. MozReview-Commit-ID: WfjMC2p2eK --HG-- extra : rebase_source : 51704fa761053e947a2c56706627c8ae903790ee extra : histedit_source : 463b2c88c28476a824bc2951e0731453016aaa2b --- dom/base/nsFrameLoader.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp index a1b614b61ca6..da25563d6a13 100644 --- a/dom/base/nsFrameLoader.cpp +++ b/dom/base/nsFrameLoader.cpp @@ -57,6 +57,7 @@ #include "nsLayoutUtils.h" #include "nsMappedAttributes.h" #include "nsView.h" +#include "nsBaseWidget.h" #include "GroupedSHistory.h" #include "PartialSHistory.h" @@ -1249,6 +1250,12 @@ nsFrameLoader::ShowRemoteFrame(const ScreenIntSize& size, return false; } + // We never want to host remote frameloaders in simple popups, like menus. + nsIWidget* widget = nsContentUtils::WidgetForContent(mOwnerContent); + if (!widget || static_cast(widget)->IsSmallPopup()) { + return false; + } + RenderFrameParent* rfp = GetCurrentRenderFrame(); if (!rfp) { return false; From dc3d84653a693787dddab1f70bcbf970c6ffdc5d Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Mon, 17 Apr 2017 13:29:56 -0700 Subject: [PATCH 12/15] Bug 1353060: Allow APZ in popup window widgets. r=kats Prior to this patchset, we never hosted remote, scrollable content in widgets other than toplevel and child windows, so all other widget types have APZ disabled. Since we now need to host remote content in popup widgets, and would like similar performance and behavior for those browsers as other remote browsers, we also need to enable APZ for those widgets too. MozReview-Commit-ID: AVDt9U5i2WK --HG-- extra : rebase_source : a9482d966b9d910fe0f989f2ff5daf49d60033c8 extra : histedit_source : 4c039c182535acdc690abbc8201db749169e9b80 --- widget/nsBaseWidget.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/widget/nsBaseWidget.cpp b/widget/nsBaseWidget.cpp index 7ef9d9713c5b..3897b0f18920 100644 --- a/widget/nsBaseWidget.cpp +++ b/widget/nsBaseWidget.cpp @@ -922,7 +922,9 @@ bool nsBaseWidget::UseAPZ() { return (gfxPlatform::AsyncPanZoomEnabled() && - (WindowType() == eWindowType_toplevel || WindowType() == eWindowType_child)); + (WindowType() == eWindowType_toplevel || + WindowType() == eWindowType_child || + (WindowType() == eWindowType_popup && !IsSmallPopup()))); } void nsBaseWidget::CreateCompositor() From 144694720e974dc8f512fe6348205713c12b47f1 Mon Sep 17 00:00:00 2001 From: David Major Date: Wed, 3 May 2017 23:26:47 -0400 Subject: [PATCH 13/15] Bug 1355559: Suppress stack walking in LdrResolveDelayLoadedAPI. r=mstange,aklotz --- mozglue/misc/StackWalk.cpp | 21 +++++++++++++++++++++ toolkit/xre/test/win/TestDllInterceptor.cpp | 13 +++++++++++++ 2 files changed, 34 insertions(+) diff --git a/mozglue/misc/StackWalk.cpp b/mozglue/misc/StackWalk.cpp index 1167e682d3d5..0527a5d4f43d 100644 --- a/mozglue/misc/StackWalk.cpp +++ b/mozglue/misc/StackWalk.cpp @@ -296,6 +296,24 @@ patched_LdrUnloadDll(HMODULE module) AutoSuppressStackWalking suppress; return stub_LdrUnloadDll(module); } + +// These pointers are disguised as PVOID to avoid pulling in obscure headers +typedef PVOID (WINAPI *LdrResolveDelayLoadedAPI_func)(PVOID ParentModuleBase, + PVOID DelayloadDescriptor, PVOID FailureDllHook, PVOID FailureSystemHook, + PVOID ThunkAddress, ULONG Flags); +static LdrResolveDelayLoadedAPI_func stub_LdrResolveDelayLoadedAPI; + +static PVOID WINAPI patched_LdrResolveDelayLoadedAPI(PVOID ParentModuleBase, + PVOID DelayloadDescriptor, PVOID FailureDllHook, PVOID FailureSystemHook, + PVOID ThunkAddress, ULONG Flags) +{ + // Prevent the stack walker from suspending this thread when + // LdrResolveDelayLoadAPI holds the RtlLookupFunctionEntry lock. + AutoSuppressStackWalking suppress; + return stub_LdrResolveDelayLoadedAPI(ParentModuleBase, DelayloadDescriptor, + FailureDllHook, FailureSystemHook, + ThunkAddress, Flags); +} #endif // STACKWALK_HAS_DLL_INTERCEPTOR #endif // _M_AMD64 @@ -388,6 +406,9 @@ EnsureWalkThreadReady() NtDllInterceptor.AddHook("LdrUnloadDll", reinterpret_cast(patched_LdrUnloadDll), (void**)&stub_LdrUnloadDll); + NtDllInterceptor.AddHook("LdrResolveDelayLoadedAPI", + reinterpret_cast(patched_LdrResolveDelayLoadedAPI), + (void**)&stub_LdrResolveDelayLoadedAPI); #endif InitializeDbgHelpCriticalSection(); diff --git a/toolkit/xre/test/win/TestDllInterceptor.cpp b/toolkit/xre/test/win/TestDllInterceptor.cpp index 9115a378470b..51fb4191bf82 100644 --- a/toolkit/xre/test/win/TestDllInterceptor.cpp +++ b/toolkit/xre/test/win/TestDllInterceptor.cpp @@ -243,6 +243,18 @@ bool TestLdrUnloadDll(void* aFunc) return patchedLdrUnloadDll(0) != 0; } +bool TestLdrResolveDelayLoadedAPI(void* aFunc) +{ + // These pointers are disguised as PVOID to avoid pulling in obscure headers + typedef PVOID (WINAPI *LdrResolveDelayLoadedAPIType)(PVOID, PVOID, PVOID, + PVOID, PVOID, ULONG); + auto patchedLdrResolveDelayLoadedAPI = + reinterpret_cast(aFunc); + // No idea how to call this API. Flags==99 is just an arbitrary number that + // doesn't crash when the other params are null. + return patchedLdrResolveDelayLoadedAPI(0, 0, 0, 0, 0, 99) == 0; +} + bool TestSetUnhandledExceptionFilter(void* aFunc) { auto patchedSetUnhandledExceptionFilter = @@ -471,6 +483,7 @@ int main() #ifdef _M_X64 TestHook(TestGetKeyState, "user32.dll", "GetKeyState") && // see Bug 1316415 TestHook(TestLdrUnloadDll, "ntdll.dll", "LdrUnloadDll") && + TestHook(TestLdrResolveDelayLoadedAPI, "ntdll.dll", "LdrResolveDelayLoadedAPI") && #endif MaybeTestHook(ShouldTestTipTsf(), TestProcessCaretEvents, "tiptsf.dll", "ProcessCaretEvents") && #ifdef _M_IX86 From ab85c1ac986e1b57b846f789129fac2942c6a3e6 Mon Sep 17 00:00:00 2001 From: Iris Hsiao Date: Thu, 4 May 2017 11:30:45 +0800 Subject: [PATCH 14/15] Backed out changeset dda520b4ed32 (bug 1360572) for Assertion failure at RasterImage.cpp --- image/FrameAnimator.cpp | 1 - image/RasterImage.cpp | 25 +++++++++++-------------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/image/FrameAnimator.cpp b/image/FrameAnimator.cpp index f96952ee1ac6..4602782af990 100644 --- a/image/FrameAnimator.cpp +++ b/image/FrameAnimator.cpp @@ -425,7 +425,6 @@ FrameAnimator::RequestRefresh(AnimationState& aState, // Advanced to the correct frame, the composited frame is now valid to be drawn. if (*currentFrameEndTime > aTime) { aState.mCompositedFrameInvalid = false; - ret.mDirtyRect = IntRect(IntPoint(0,0), mSize); } MOZ_ASSERT(!aState.mIsCurrentlyDecoded || !aState.mCompositedFrameInvalid); diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp index f89573b1d300..f5d62d6259d1 100644 --- a/image/RasterImage.cpp +++ b/image/RasterImage.cpp @@ -1262,20 +1262,17 @@ RasterImage::Decode(const IntSize& aSize, mSourceBuffer, mSize, decoderFlags, surfaceFlags); // We may not be able to send an invalidation right here because of async - // notifications but that shouldn't be a problem because we shouldn't be - // getting a non-empty rect back from UpdateState. This is because UpdateState - // will only return a non-empty rect if we are currently decoded, or the - // animation is finished. We can't be decoded because we are creating a decoder - // here. If the animation is finished then the composited frame would have - // been valid when the animation finished, and it's not possible to mark - // the composited frame as invalid when the animation is finished. So - // the composited frame can't change from invalid to valid in this UpdateState - // call, and hence no rect can be returned. -#ifdef DEBUG - gfx::IntRect rect = -#endif - mAnimationState->UpdateState(mAnimationFinished, this, mSize); - MOZ_ASSERT(rect.IsEmpty()); + // notifications but that's not a problem because the first frame + // invalidation (when it comes) will invalidate for us. So we can ignore + // the return value of UpdateState. This also handles the invalidation + // from setting the composited frame as valid below. + mAnimationState->UpdateState(mAnimationFinished, this, mSize); + // If the animation is finished we can draw right away because we just draw + // the final frame all the time from now on. See comment in + // AnimationState::UpdateState. + if (mAnimationFinished) { + mAnimationState->SetCompositedFrameInvalid(false); + } } else { task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this), mSourceBuffer, mSize, aSize, From f11ddd537bbb14af63d7b33b93ac2b8448f3a3d4 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 May 2017 16:54:25 -0400 Subject: [PATCH 15/15] Bug 1361274. There's no need to update link :visited state when doing querySelectorAll, since querySelectorAll ignores that state anyway. r=smaug In our current setup, in which links with an href attribute always match either :link or :visited, no matter whether that attribute's value is a valid URI, changes to the attribute always put the element into either the "match nothing" state or the "match :link" state, via calls to Link::ResetLinkState. The only thing FlushPendingLinkUpdates is needed for is (lazily, in case it turns out to not be needed because the element got removed from the DOM anyway) registering a history observer to switch the link state to :visited as needed. This means that selector matching consumers that would never expose :visited state to start with don't need to worry about calling FlushPendingLinkUpdates. --- dom/base/Element.cpp | 2 -- dom/base/nsIDocument.h | 16 ++++++++++++++-- dom/base/nsINode.cpp | 1 - dom/html/HTMLContentElement.cpp | 1 - layout/inspector/inDOMUtils.cpp | 4 ++-- layout/style/nsComputedDOMStyle.cpp | 2 -- layout/style/nsRuleProcessorData.h | 10 ++++++++++ 7 files changed, 26 insertions(+), 10 deletions(-) diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp index 2ea545f87c4c..edaf0bbdc3be 100644 --- a/dom/base/Element.cpp +++ b/dom/base/Element.cpp @@ -3210,7 +3210,6 @@ Element::Closest(const nsAString& aSelector, ErrorResult& aResult) // is a pseudo-element-only selector that matches nothing. return nullptr; } - OwnerDoc()->FlushPendingLinkUpdates(); TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited, OwnerDoc(), @@ -3238,7 +3237,6 @@ Element::Matches(const nsAString& aSelector, ErrorResult& aError) return false; } - OwnerDoc()->FlushPendingLinkUpdates(); TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited, OwnerDoc(), diff --git a/dom/base/nsIDocument.h b/dom/base/nsIDocument.h index 8cdfa88e9cb7..5b4955952fd2 100644 --- a/dom/base/nsIDocument.h +++ b/dom/base/nsIDocument.h @@ -2464,8 +2464,20 @@ public: // Add aLink to the set of links that need their status resolved. void RegisterPendingLinkUpdate(mozilla::dom::Link* aLink); - // Update state on links in mLinksToUpdate. This function must - // be called prior to selector matching. + // Update state on links in mLinksToUpdate. This function must be called + // prior to selector matching that needs to differentiate between :link and + // :visited. In particular, it does _not_ need to be called before doing any + // selector matching that uses TreeMatchContext::eNeverMatchVisited. The only + // reason we haven't moved all calls to this function entirely inside the + // TreeMatchContext constructor is to not call it all the time during various + // style system and frame construction operations (though it would likely be a + // no-op for all but the first call). + // + // XXXbz Does this really need to be called before selector matching? All it + // will do is ensure all the links involved are registered to observe history, + // which won't synchronously change their state to :visited anyway! So + // calling this won't affect selector matching done immediately afterward, as + // far as I can tell. void FlushPendingLinkUpdates(); void FlushPendingLinkUpdatesFromRunnable(); diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp index b60a13bd851c..0133e849763e 100644 --- a/dom/base/nsINode.cpp +++ b/dom/base/nsINode.cpp @@ -2806,7 +2806,6 @@ FindMatchingElements(nsINode* aRoot, nsCSSSelectorList* aSelectorList, T &aList, TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited, doc, TreeMatchContext::eNeverMatchVisited); - doc->FlushPendingLinkUpdates(); AddScopeElements(matchingContext, aRoot); // Fast-path selectors involving IDs. We can only do this if aRoot diff --git a/dom/html/HTMLContentElement.cpp b/dom/html/HTMLContentElement.cpp index 77309a10b7b6..867a5fe18c4b 100644 --- a/dom/html/HTMLContentElement.cpp +++ b/dom/html/HTMLContentElement.cpp @@ -295,7 +295,6 @@ HTMLContentElement::Match(nsIContent* aContent) TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited, doc, TreeMatchContext::eNeverMatchVisited); - doc->FlushPendingLinkUpdates(); matchingContext.SetHasSpecifiedScope(); matchingContext.AddScopeElement(host->AsElement()); diff --git a/layout/inspector/inDOMUtils.cpp b/layout/inspector/inDOMUtils.cpp index 56a054396ba5..adab3782d4bc 100644 --- a/layout/inspector/inDOMUtils.cpp +++ b/layout/inspector/inDOMUtils.cpp @@ -513,8 +513,8 @@ inDOMUtils::SelectorMatchesElement(nsIDOMElement* aElement, sel->RemoveRightmostSelector(); } - element->OwnerDoc()->FlushPendingLinkUpdates(); - // XXXbz what exactly should we do with visited state here? + // XXXbz what exactly should we do with visited state here? If we ever start + // caring about it, remember to do FlushPendingLinkUpdates(). TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited, element->OwnerDoc(), diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp index e3c578c42ae6..076733a882ac 100644 --- a/layout/style/nsComputedDOMStyle.cpp +++ b/layout/style/nsComputedDOMStyle.cpp @@ -758,8 +758,6 @@ nsComputedDOMStyle::UpdateCurrentStyleSources(bool aNeedsLayoutFlush) return; } - document->FlushPendingLinkUpdates(); - // Flush _before_ getting the presshell, since that could create a new // presshell. Also note that we want to flush the style on the document // we're computing style in, not on the document mContent is in -- the two diff --git a/layout/style/nsRuleProcessorData.h b/layout/style/nsRuleProcessorData.h index 9042fe3c9a12..6ace4cb5a093 100644 --- a/layout/style/nsRuleProcessorData.h +++ b/layout/style/nsRuleProcessorData.h @@ -370,6 +370,13 @@ struct MOZ_STACK_CLASS TreeMatchContext { // for an HTML5 scoped style sheet. bool mForScopedStyle; + // An enum that communicates the consumer's intensions for this + // TreeMatchContext in terms of :visited handling. eNeverMatchVisited means + // that this TreeMatchContext's VisitedHandlingType will always be + // eRelevantLinkUnvisited (in other words, this value will be passed to the + // constructor and ResetForVisitedMatching() will never be called). + // eMatchVisitedDefault doesn't communicate any information about the current + // or future VisitedHandlingType of this TreeMatchContext. enum MatchVisited { eNeverMatchVisited, eMatchVisitedDefault @@ -404,6 +411,9 @@ struct MOZ_STACK_CLASS TreeMatchContext { if (loadContext) { mUsingPrivateBrowsing = loadContext->UsePrivateBrowsing(); } + } else { + MOZ_ASSERT(aVisitedHandling == nsRuleWalker::eRelevantLinkUnvisited, + "You promised you'd never try to match :visited!"); } }