Animated svg images used as backgrounds doesn't animate. It starts animating again while hovering the image in firebug. Last good nightly: 2013-09-04 First bad nightly: 2013-09-05 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e3785e299ab6&tochange=77ed46bf4c1a

This affects release too so it looks like it was something that was uplifted.

jwatt, Giji suggested I'll ask you if you have a clue about this.

Last good Aurora: 2013-09-06 00:40:01 First bad Aurora: 2013-09-07 00:40:02 Pushlog: http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=b0bc9c628c80&tochange=dd2d818af48a

I can reproduce using attachment 828542 [details], with shift+Reload (to force us to skip the image cache). The first instance of the throbber doesn't appear to be sending invalidations correctly. When I force a repaint by e.g. zooming in or changing tabs, the throbber repaints with an updated position, but it doesn't animate on its own. If I do a simple reload, then it works as expected.

I've made some progress on this: RequestBehaviour::GetImage imgRequestProxy::GetImage imgRequestProxy::GetImage nsLayoutUtils::RegisterImageRequestIfAnimated mozilla::css::ImageLoader::AssociateRequestToFrame nsFrame::DidSetStyleContext nsFrame::Init nsSplittableFrame::Init nsContainerFrame::Init nsBlockFrame::Init nsCSSFrameConstructor::InitAndRestoreFrame nsCSSFrameConstructor::ConstructBlock nsCSSFrameConstructor::ConstructNonScrollableBlock nsCSSFrameConstructor::ConstructFrameFromItemInternal nsCSSFrameConstructor::ConstructFramesFromItem nsCSSFrameConstructor::ConstructFramesFromItemList nsCSSFrameConstructor::ContentAppended nsCSSFrameConstructor::CreateNeededFrames nsCSSFrameConstructor::CreateNeededFrames nsCSSFrameConstructor::CreateNeededFrames PresShell::FlushPendingNotifications nsRefreshDriver::Tick RequestBehaviour::GetImage returns nullptr, so imgRequestProxy::GetImage tries GetOwner()->mImage, finds it's also nullptr, so returns NS_ERROR_FAILURE. As a result RegisterImageRequestIfAnimated doesn't call nsRefreshDriver::AddImageRequest which would otherwise call mRequests.PutEntry(aRequest). The failure to add a request to nsRefreshDriver::mRequests means nsRefreshDriver::Tick doesn't call RequestRefresh for the image, and hence no animation. I'll dig some more to try and found why the imgRequestProxy isn't in the state that we need.

On first load we create the imgStatusTracker that holds the mozilla::image::Image that the imgRequestProxy is going to return when imgIRequest::GetImage is called on it (returning something like imgRequestProxy->mBehaviour->GetStatusTracker()->imgStatusTracker->mImage). At this point imgStatusTracker::mImage is set to null: imgStatusTracker::imgStatusTracker imgStatusTracker::imgStatusTracker imgRequest::imgRequest imgRequest::imgRequest NewRequestAndEntry imgLoader::LoadImage nsContentUtils::LoadImage mozilla::css::ImageLoader::LoadImage mozilla::css::ImageValue::ImageValue mozilla::css::ImageValue::ImageValue nsCSSValue::StartImageLoad TryToStartImageLoadOnValue TryToStartImageLoad TryToStartImageLoad nsCSSCompressedDataBlock::MapRuleInfoInto mozilla::css::Declaration::MapNormalRuleInfoInto mozilla::css::StyleRule::MapRuleInfoInto nsRuleNode::WalkRuleTree nsRuleNode::GetStyleBackground nsStyleContext::DoGetStyleBackground nsStyleContext::StyleBackground nsStyleContext::StartBackgroundImageLoads nsCSSFrameConstructor::ConstructFramesFromItem (The imgRequestProxy is created in the imgLoader::LoadImage call in that stack and returned via an outparam. Next, on returning to nsRuleNode::WalkRuleTree, the imgRequestProxy is assigned to the nsStyleImage under this stack: nsStyleImage::SetImageData SetStyleImage BackgroundItemComputer<nsCSSValueList, nsStyleImage>::ComputeValue void SetBackgroundList<nsStyleImage> nsRuleNode::ComputeBackgroundData nsRuleNode::WalkRuleTree nsRuleNode::GetStyleBackground nsStyleContext::DoGetStyleBackground nsStyleContext::StyleBackground nsStyleContext::StartBackgroundImageLoads nsCSSFrameConstructor::ConstructFramesFromItem This is the nsStyleImage that nsFrame::DidSetStyleContext passes to ImageLoader::AssociateRequestToFrame in the stack in comment 6. Next we hit nsLayoutUtils::RegisterImageRequestIfAnimated and fail to call nsRefreshDriver::AddImageRequest as described in comment 6. Next, too late, we set imgStatusTracker::mImage to something non-null: imgStatusTracker::SetImage imgStatusTrackerInit::imgStatusTrackerInit imgStatusTrackerInit::imgStatusTrackerInit mozilla::image::VectorImage::VectorImage mozilla::image::VectorImage::VectorImage mozilla::image::ImageFactory::CreateVectorImage mozilla::image::ImageFactory::CreateImage imgRequest::OnDataAvailable ProxyListener::OnDataAvailable nsBaseChannel::OnDataAvailable _ZThn128_N13nsBaseChannel15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreamyj nsInputStreamPump::OnStateTransfer nsInputStreamPump::OnInputStreamReady _ZThn8_N17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream nsInputStreamReadyEvent::Run nsThread::ProcessNextEvent

On a soft refresh we create a new imgStatusTracker as in the first stack in comment 7. However, in the nsStyleImage::SetImageData given by the second stack the new imgIRequest* object passed in uses exactly the same imgStatusTracker (returned by aImage->mBehaviour->GetStatusTracker()) that existed during the initial load, and that imgStatusTracker holds the exact same mozilla::image::Image object that was created during the initial load. nsLayoutUtils::RegisterImageRequestIfAnimated then gets that old Image when it calls aRequest->GetImage(), and passes it to nsRefreshDriver::AddImageRequest making the new document's refresh driver an observer of the previous document's Image. At it happens that seems to "work" in so far as the old VectorImage stays alive, animates, and can be used by the new document.

Jonathan, I think what you're seeing is the expected behavior, though I'm not 100% sure. What I expect should happen is that on a soft reload, we should validate the cached image, which will involve calling NewRequestAndEntry from imgCacheValidator's constructor. (Not the same stack as in comment 7, though there is a resemblance.) The server reports that the image has not changed, and we then decide to use the old imgRequest object we cached from before. Thus, your observation that the old imgRequest and Image is reused is both true and expected. It works, without quotes. =) This is by design. However, there is definitely a bug here, though I'm still not sure of the precise details. RasterImages let their observers know that they have become animated (since they don't know if they will be until the second frame is available) by calling imgStatusTracker::RecordImageIsAnimated/SendImageIsAnimated. VectorImage doesn't do that, and that may be the problem.

Ah, okay, good to know there's not an additional bug here. Thanks, Seth. So the case that we need to fix is the case where the image becomes available _after_ SetStyleContext is called to set the style context for the element that's using that image as its background (where it calls AssociateRequestToFrame before the imgRequestProxy has the image). Calling imgStatusTracker::RecordImageIsAnimated from VectorImage::StartAnimation is not sufficient to fix this, BTW. (Not too surprisingly, since VectorImage needs to be registered with the refresh driver.) I'm still not clear on the right fix though.

What's the status on this?

It's waiting for me or someone else to find some time to pick up where I left off and figure out how this should be fixed. I'm not likely to have time this week, and possibly not before January. :(

This looks like an ImageLib bug to me. I'm changing the component.

Any update on this? This makes svg animations close to unusable in firefox.

Seth, do you know who might have time to look into this?

This patch is actually a simplified version of CJ Ku's WIP patch. It seems that imgRequest is put to animation registration before it complete loading (So PrograssTracker::GetImage() returns nullptr), move it to Notify can correct the order. I'm not sure this is correct solution so I'm still finding out why only SVG background has this issue since AssociateImageRequestToFrame() is called in many paths.

Comment on attachment 8756744 [details] [diff] [review] Bug935888 - Move RegisterImageRequestIfAnimated from AssociateImageRequestToFrame() to Notify() in ImageLoader Review of attachment 8756744 [details] [diff] [review]: ----------------------------------------------------------------- I think the bug is caused because we register it to refresh driver before it complete loading. When HTTP side has done loading the image, imgRequest::OnDataAvailable will be called and we have the image. We call RegisterImageRequestIfAnimated before it, so we get nullptr and fail. When we refresh, the image has been loaded so it's successfully registered so we can see expected result. This patch postpones RegisterImageRequestIfAnimated from AssociateImageToFrame to Notify which can correct the order but I am not sure whether it's a good solution or not.

A lot of code related to this problem has changed *drastically* in the last 2 and a half years; it probably needs to be debugged again. Before f+'ing or r+'ing anything, we really need to understand why this code does not result in the image being registered with the refresh driver: https://dxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.cpp?from=layout%2Fstyle%2FImageLoader.cpp#422 I don't understand why this isn't working. This should be simple.

Michael, can you please figure out what the answer is to my question in comment 21? I suspect answering that question will lead us to the right fix.

OK, I will investigate this part of code, thanks for your suggestion :)

I traced the ImageLoader::Notify part, found that ImageLoader::OnImageIsAnimated does not execute during the whole svg display process. I think we actually register SVG in CSS background to RefreshDriver in ImageLoader::AssociateRequestToFrame. https://dxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.cpp#45 This method calls nsLayoutUtils::RegisterImageRequestIfAnimated. The problem is that inside nsLayoutUtils::RegisterImageRequestIfAnimated, there is a check to determine whether the image is animated or not which needs to obtain the image info in corresponding imgIRequest. However, the imgIRequest returns nullptr so the upcoming registration is skipped. https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#7937 It's a race condition that the register process is invoked before the image is available. The simplest workaround is using nsLayoutUtils::RegisterImageRequest instead of nsLayoutUtils::RegisterImageRequestIfAnimated to bypass the check. For a better solution, I think we should buffer these registration requests until necessary data is available.

(In reply to Michael Leu[:lenzak800](UTC+8) from comment #24) > I traced the ImageLoader::Notify part, found that > ImageLoader::OnImageIsAnimated does not execute during the whole svg display > process. That's enough to identify the problem. Thanks so much for debugging this, Michael! The required patch here is basically a one-liner so I'm just going to go ahead and write it.

Here's the patch. Daniel, I'm going to file a followup to add not only a test for this issue, but also a general unit test suite for VectorImage. It's not too hard to put together a simple one but it's a big enough task that it shouldn't be a part of this bug. We would've caught this trivially if we had the same kind of GTest suite for VectorImage that we have for RasterImage.

Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf7ba94aa334

Comment on attachment 8765762 [details] [diff] [review] Set FLAG_IS_ANIMATED progress bit for VectorImages when appropriate. Review of attachment 8765762 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Many thanks to both Michael & Seth for figuring this out!

Pushed by mfowler@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/49b3e41bb16b Set FLAG_IS_ANIMATED progress bit for VectorImages when appropriate. r=dholbert

Thanks for the review, Daniel!

I have reproduced the issue using the testcase provided in comment 1, on Firefox 28.0 RC using Windows 10 Pro x64. The issue is now fixed on Beta 50.0b3 Build 1, across platforms Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Yosemite 10.10.4

Thank you, Carmen!