From 2196ea568915bbb5b5ef0de594e663c67ba1d416 Mon Sep 17 00:00:00 2001 From: Till Schneidereit Date: Sat, 25 Mar 2017 17:43:44 -0700 Subject: [PATCH 1/3] Bug 1339999 - Properly handle OOM during exception throwing in all Promise code. r=arai MozReview-Commit-ID: 2uZgTZAKYnK --- js/src/builtin/Promise.cpp | 41 ++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/js/src/builtin/Promise.cpp b/js/src/builtin/Promise.cpp index 9e7c68a6f452..63acb055f9d8 100644 --- a/js/src/builtin/Promise.cpp +++ b/js/src/builtin/Promise.cpp @@ -131,6 +131,20 @@ NewPromiseAllDataHolder(JSContext* cx, HandleObject resultPromise, HandleValue v return dataHolder; } +/** + * Wrapper for GetAndClearException that handles cases where no exception is + * pending, but an error occurred. This can be the case if an OOM was + * encountered while throwing the error. + */ +static bool +MaybeGetAndClearException(JSContext* cx, MutableHandleValue rval) +{ + if (!cx->isExceptionPending()) + return false; + + return GetAndClearException(cx, rval); +} + static MOZ_MUST_USE bool RunResolutionFunction(JSContext *cx, HandleObject resolutionFun, HandleValue result, ResolutionMode mode, HandleObject promiseObj); @@ -142,13 +156,9 @@ static MOZ_MUST_USE bool RunResolutionFunction(JSContext *cx, HandleObject resol static bool AbruptRejectPromise(JSContext *cx, CallArgs& args, HandleObject promiseObj, HandleObject reject) { - // Not much we can do about uncatchable exceptions, so just bail. - if (!cx->isExceptionPending()) - return false; - // Step 1.a. RootedValue reason(cx); - if (!GetAndClearException(cx, &reason)) + if (!MaybeGetAndClearException(cx, &reason)) return false; if (!RunResolutionFunction(cx, reject, reason, RejectMode, promiseObj)) @@ -351,7 +361,8 @@ ResolvePromiseInternal(JSContext* cx, HandleObject promise, HandleValue resoluti JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_CANNOT_RESOLVE_PROMISE_WITH_ITSELF); RootedValue selfResolutionError(cx); - MOZ_ALWAYS_TRUE(GetAndClearException(cx, &selfResolutionError)); + if (!MaybeGetAndClearException(cx, &selfResolutionError)) + return false; // Step 6.b. return RejectMaybeWrappedPromise(cx, promise, selfResolutionError); @@ -364,7 +375,7 @@ ResolvePromiseInternal(JSContext* cx, HandleObject promise, HandleValue resoluti // Step 9. if (!status) { RootedValue error(cx); - if (!GetAndClearException(cx, &error)) + if (!MaybeGetAndClearException(cx, &error)) return false; return RejectMaybeWrappedPromise(cx, promise, error); @@ -899,9 +910,7 @@ PromiseReactionJob(JSContext* cx, unsigned argc, Value* vp) args2[0].set(argument); if (!Call(cx, handlerVal, UndefinedHandleValue, args2, &handlerResult)) { resolutionMode = RejectMode; - // Not much we can do about uncatchable exceptions, so just bail - // for those. - if (!cx->isExceptionPending() || !GetAndClearException(cx, &handlerResult)) + if (!MaybeGetAndClearException(cx, &handlerResult)) return false; } } @@ -978,7 +987,7 @@ PromiseResolveThenableJob(JSContext* cx, unsigned argc, Value* vp) if (Call(cx, then, thenable, args2, &rval)) return true; - if (!GetAndClearException(cx, &rval)) + if (!MaybeGetAndClearException(cx, &rval)) return false; FixedInvokeArgs<1> rejectArgs(cx); @@ -1321,9 +1330,7 @@ PromiseObject::create(JSContext* cx, HandleObject executor, HandleObject proto / // Step 10. if (!success) { RootedValue exceptionVal(cx); - // Not much we can do about uncatchable exceptions, so just bail - // for those. - if (!cx->isExceptionPending() || !GetAndClearException(cx, &exceptionVal)) + if (!MaybeGetAndClearException(cx, &exceptionVal)) return nullptr; FixedInvokeArgs<1> args(cx); @@ -2125,13 +2132,9 @@ js::CreatePromiseObjectForAsync(JSContext* cx, HandleValue generatorVal) MOZ_MUST_USE bool js::AsyncFunctionThrown(JSContext* cx, Handle resultPromise) { - // Not much we can do about uncatchable exceptions, so just bail. - if (!cx->isExceptionPending()) - return false; - // Step 3.f. RootedValue exc(cx); - if (!GetAndClearException(cx, &exc)) + if (!MaybeGetAndClearException(cx, &exc)) return false; if (!RejectMaybeWrappedPromise(cx, resultPromise, exc)) From 1203da31b5ce2f8259b018ff0e307268e0b914ec Mon Sep 17 00:00:00 2001 From: Ehsan Akhgari Date: Sat, 25 Mar 2017 22:19:20 -0400 Subject: [PATCH 2/3] Bug 1350090 - Turn off the spammy warning that goes off every time we create an about:blank content viewer; r=mystor --- extensions/cookie/nsPermissionManager.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp index 99a3f769c096..71af2ffc40ea 100644 --- a/extensions/cookie/nsPermissionManager.cpp +++ b/extensions/cookie/nsPermissionManager.cpp @@ -87,7 +87,11 @@ nsresult GetOriginFromPrincipal(nsIPrincipal* aPrincipal, nsACString& aOrigin) { nsresult rv = aPrincipal->GetOriginNoSuffix(aOrigin); - NS_ENSURE_SUCCESS(rv, rv); + // The principal may belong to the about:blank content viewer, so this can be + // expected to fail. + if (NS_FAILED(rv)) { + return rv; + } nsAutoCString suffix; rv = aPrincipal->GetOriginSuffix(suffix); From 1614a73baddcc08968a34872168f9de10249f5d3 Mon Sep 17 00:00:00 2001 From: Timothy Nikkel Date: Sun, 26 Mar 2017 00:04:53 -0500 Subject: [PATCH 3/3] Bug 1343341. Rewrite animation state updating to derive new state purely based on SurfaceCache and RasterImage::mAnimationFinished. r=aosmond If the SurfaceCache discards our frames on another thread, the runnable that notifies us of that discard could race with a decode complete notification. So we can't rely on any ordering of SetDiscarded and NotifyDecodeComplete. Thus we must derive our state purely from the SurfaceCache (and mAnimationFinished from RasterImage). We also update the image state in RequestRefresh (the main place where we use the state that is updated). The other main place we use the state is GetCompositedFrame, but we don't update the state there. It should be fine because the only time this might lag behind reality is if the frames are discarded, and it should be fine to continue drawing the composited frame until the discard notification arrives. The way that we tell that an animated image has all of its frames complete in the surface cache is less than ideal. --- image/FrameAnimator.cpp | 90 ++++++++++++++++++++++++++++++----------- image/FrameAnimator.h | 23 +++++++---- image/RasterImage.cpp | 13 +++--- 3 files changed, 89 insertions(+), 37 deletions(-) diff --git a/image/FrameAnimator.cpp b/image/FrameAnimator.cpp index 663d5b6680fa..36d9a12d4f02 100644 --- a/image/FrameAnimator.cpp +++ b/image/FrameAnimator.cpp @@ -26,36 +26,80 @@ namespace image { /////////////////////////////////////////////////////////////////////////////// void -AnimationState::NotifyDecodeComplete() +AnimationState::UpdateState(bool aAnimationFinished, + RasterImage *aImage, + const gfx::IntSize& aSize) { - // If we weren't discarded before the decode finished then mark ourselves as - // currently decoded. - if (!mDiscarded) { - mIsCurrentlyDecoded = true; + LookupResult result = + SurfaceCache::Lookup(ImageKey(aImage), + RasterSurfaceKey(aSize, + DefaultSurfaceFlags(), + PlaybackType::eAnimated)); + UpdateStateInternal(result, aAnimationFinished); +} + +void +AnimationState::UpdateStateInternal(LookupResult& aResult, + bool aAnimationFinished) +{ + // Update mDiscarded and mIsCurrentlyDecoded. + if (aResult.Type() == MatchType::NOT_FOUND) { + // no frames, we've either been discarded, or never been decoded before. + mDiscarded = mHasBeenDecoded; + mIsCurrentlyDecoded = false; + } else if (aResult.Type() == MatchType::PENDING) { + // no frames yet, but a decoder is or will be working on it. + mDiscarded = false; + mIsCurrentlyDecoded = false; + } else { + MOZ_ASSERT(aResult.Type() == MatchType::EXACT); + mDiscarded = false; + + // If mHasBeenDecoded is true then we know the true total frame count and + // we can use it to determine if we have all the frames now so we know if + // we are currently fully decoded. + // If mHasBeenDecoded is false then we'll get another UpdateState call + // when the decode finishes. + if (mHasBeenDecoded) { + Maybe frameCount = FrameCount(); + MOZ_ASSERT(frameCount.isSome()); + aResult.Surface().Seek(*frameCount - 1); + if (aResult.Surface() && aResult.Surface()->IsFinished()) { + mIsCurrentlyDecoded = true; + } else { + mIsCurrentlyDecoded = false; + } + } + } + + // Update the value of mCompositedFrameInvalid. + if (mIsCurrentlyDecoded || aAnimationFinished) { // Animated images that have finished their animation (ie because it is a // finite length animation) don't have RequestRefresh called on them, and so // mCompositedFrameInvalid would never get cleared. We clear it here (and // also in RasterImage::Decode when we create a decoder for an image that // has finished animated so it can display sooner than waiting until the - // decode completes). This is safe to do for images that aren't finished - // animating because before we paint the refresh driver will call into us - // to advance to the correct frame, and that will succeed because we have - // all the frames. + // decode completes). We also do it if we are fully decoded. This is safe + // to do for images that aren't finished animating because before we paint + // the refresh driver will call into us to advance to the correct frame, + // and that will succeed because we have all the frames. mCompositedFrameInvalid = false; + } else if (aResult.Type() == MatchType::NOT_FOUND || + aResult.Type() == MatchType::PENDING) { + if (mHasBeenDecoded) { + MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); + mCompositedFrameInvalid = true; + } } - mHasBeenDecoded = true; + // Otherwise don't change the value of mCompositedFrameInvalid, it will be + // updated by RequestRefresh. } void -AnimationState::SetDiscarded(bool aDiscarded) +AnimationState::NotifyDecodeComplete() { - if (aDiscarded) { - MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); - mIsCurrentlyDecoded = false; - mCompositedFrameInvalid = true; - } - mDiscarded = aDiscarded; + mHasBeenDecoded = true; } void @@ -307,7 +351,9 @@ FrameAnimator::AdvanceFrame(AnimationState& aState, } RefreshResult -FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime) +FrameAnimator::RequestRefresh(AnimationState& aState, + const TimeStamp& aTime, + bool aAnimationFinished) { // By default, an empty RefreshResult. RefreshResult ret; @@ -326,12 +372,8 @@ FrameAnimator::RequestRefresh(AnimationState& aState, const TimeStamp& aTime) DefaultSurfaceFlags(), PlaybackType::eAnimated)); - if (!result) { - if (result.Type() == MatchType::NOT_FOUND) { - // No surface, and nothing pending, must have been discarded but - // we haven't been notified yet. - aState.SetDiscarded(true); - } + aState.UpdateStateInternal(result, aAnimationFinished); + if (aState.IsDiscarded() || !result) { return ret; } diff --git a/image/FrameAnimator.h b/image/FrameAnimator.h index dde7abb715bc..44b5a52e7cac 100644 --- a/image/FrameAnimator.h +++ b/image/FrameAnimator.h @@ -39,6 +39,19 @@ public: , mDiscarded(false) { } + /** + * Call this whenever a decode completes, a decode starts, or the image is + * discarded. It will update the internal state. Specifically mDiscarded, + * mCompositedFrameInvalid, and mIsCurrentlyDecoded. + */ + void UpdateState(bool aAnimationFinished, + RasterImage *aImage, + const gfx::IntSize& aSize); +private: + void UpdateStateInternal(LookupResult& aResult, + bool aAnimationFinished); + +public: /** * Call when a decode of this image has been completed. */ @@ -49,12 +62,6 @@ public: */ bool GetHasBeenDecoded() { return mHasBeenDecoded; } - /** - * Call this with true when this image is discarded. Call this with false - * when a decoder is created to decode the image. - */ - void SetDiscarded(bool aDiscarded); - /** * Returns true if this image has been discarded and a decoded has not yet * been created to redecode it. @@ -276,7 +283,9 @@ public: * Returns the result of that blending, including whether the current frame * changed and what the resulting dirty rectangle is. */ - RefreshResult RequestRefresh(AnimationState& aState, const TimeStamp& aTime); + RefreshResult RequestRefresh(AnimationState& aState, + const TimeStamp& aTime, + bool aAnimationFinished); /** * Get the full frame for the current frame of the animation (it may or may diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp index e7a576a77d35..d7e424e9215c 100644 --- a/image/RasterImage.cpp +++ b/image/RasterImage.cpp @@ -173,7 +173,7 @@ RasterImage::RequestRefresh(const TimeStamp& aTime) RefreshResult res; if (mAnimationState) { MOZ_ASSERT(mFrameAnimator); - res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime); + res = mFrameAnimator->RequestRefresh(*mAnimationState, aTime, mAnimationFinished); } if (res.mFrameAdvanced) { @@ -450,7 +450,7 @@ RasterImage::OnSurfaceDiscarded(const SurfaceKey& aSurfaceKey) if (animatedFramesDiscarded && NS_IsMainThread()) { MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); - mAnimationState->SetDiscarded(true); + mAnimationState->UpdateState(mAnimationFinished, this, mSize); // We don't need OnSurfaceDiscardedInternal to handle the animated frames // being discarded because we just did. animatedFramesDiscarded = false; @@ -471,7 +471,7 @@ RasterImage::OnSurfaceDiscardedInternal(bool aAnimatedFramesDiscarded) if (aAnimatedFramesDiscarded && mAnimationState) { MOZ_ASSERT(gfxPrefs::ImageMemAnimatedDiscardable()); - mAnimationState->SetDiscarded(true); + mAnimationState->UpdateState(mAnimationFinished, this, mSize); } if (mProgressTracker) { @@ -1081,7 +1081,7 @@ RasterImage::Discard() SurfaceCache::RemoveImage(ImageKey(this)); if (mAnimationState) { - mAnimationState->SetDiscarded(true); + mAnimationState->UpdateState(mAnimationFinished, this, mSize); } // Notify that we discarded. @@ -1253,10 +1253,10 @@ RasterImage::Decode(const IntSize& aSize, task = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this), mSourceBuffer, mSize, decoderFlags, surfaceFlags); - mAnimationState->SetDiscarded(false); + 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::NotifyDecodeComplete. + // AnimationState::UpdateState. if (mAnimationFinished) { mAnimationState->SetCompositedFrameInvalid(false); } @@ -1717,6 +1717,7 @@ RasterImage::NotifyDecodeComplete(const DecoderFinalStatus& aStatus, // We've finished a full decode of all animation frames and our AnimationState // has been notified about them all, so let it know not to expect anymore. mAnimationState->NotifyDecodeComplete(); + mAnimationState->UpdateState(mAnimationFinished, this, mSize); } // Do some telemetry if this isn't a metadata decode.