935888 - Animated svg background doesn't update (particularly on first load / shift+reload)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Cork]

Attachment: Chromiumthrobber.svg

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

Comment 1

[Cork]

Attachment: testcase

Comment 2

[Cork]

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

Comment 3

[Cork]

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

Comment 4

[Cork]

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

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Jonathan Watt [:jwatt]]

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.

Comment 7

[Jonathan Watt [:jwatt]]

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

Comment 8

[Jonathan Watt [:jwatt]]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

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.

Comment 11

[Cork]

What's the status on this?

Comment 12

[Jonathan Watt [:jwatt]]

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. :(

Comment 13

[Seth Fowler [:seth] [:s2h]]

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

Comment 14

[Cork]

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

Comment 15

[Tobias Oelgarte]

Another year is gone. Still no progress at all. With SVG getting more and more relevant this really hurts. Firefox is more or less the only browser that still isn't able to use animated SVG background images. :-(

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[u459114]

Attachment: WIP

Comment 18

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug935888 - Move RegisterImageRequestIfAnimated from AssociateImageRequestToFrame() to Notify() in ImageLoader

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 19

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug935888 - Move RegisterImageRequestIfAnimated from AssociateImageRequestToFrame() to Notify() in ImageLoader

Comment 20

[Michael Leu[:Lenzak](UTC+8)]

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.

Comment 21

[Seth Fowler [:seth] [:s2h]]

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.

Comment 22

[Seth Fowler [:seth] [:s2h]]

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.

Comment 23

[Michael Leu[:Lenzak](UTC+8)]

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

Comment 24

[Michael Leu[:Lenzak](UTC+8)]

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.

Comment 25

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 26

[Seth Fowler [:seth] [:s2h]]

Attachment: Set FLAG_IS_ANIMATED progress bit for VectorImages when appropriate.

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.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Try job here:

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

Comment 28

[Daniel Holbert [:dholbert]]

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!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b3e41bb16b
Set FLAG_IS_ANIMATED progress bit for VectorImages when appropriate. r=dholbert

Comment 30

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Daniel!

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49b3e41bb16b

Comment 34

[Carmen]

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

Comment 35

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you, Carmen!