1024343 - Gif stops animating when loading tab is dragged to a new window

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[tashmore]

Attachment: video demonstrating the bug

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140609030202

Steps to reproduce:

Start Nightly 32.0a1 (2014-06-09). Open a new window. Enter the URL of a web page containing an animated gif. Press enter. While the page loads, drag its tab to a new window.


Actual results:

When the tab is moved to the new window, the gif stops animating. Refreshing the page causes the gif to resume animating.


Expected results:

The gif should continue animating when the tab is moved to a new window.

Comment 1

[Paul Silaghi, QA [:pauly]]

(In reply to tashmore from comment #0)
> While the page loads, drag its tab to a new window.
Confirmed, but only while the gif is loading.
33.0a1 (2014-06-12), Win 7 x64

Comment 2

[Mihai Volmer [:mihaivolmer]]

The problem has something to do with the mAnimationConsumers and mAnimating members from the RasterImage object. When the bug triggers, they become 0 and false. I am still investigating why this happens.
Moreover, when I have two tabs opened that use the same RasterImage (so mAnimationConsumers=2), the bug only triggers when I hard-refresh (Ctrl-F5) one of them, thus creating a new RasterImage with mAnimationConsumers=0 and mAnimating=false. The first one is still animating correctly with mAnimationConsumers=1 and mAnimating=true.
If I normally refresh (F5) the bug triggers if I have only one tab opened with that RasterImage.

Comment 3

[Mihai Volmer [:mihaivolmer]]

Attachment: Forces the document to animate

Comment 4

[Mihai Volmer [:mihaivolmer]]

Comment on attachment 8636973 [details] [diff] [review]
Forces the document to animate

When the document is being refreshed and the SwapWithOtherLoader() method is called, the document ends up with mAnimatingImages=false. This is what causes the bug. With this patch, I make sure that SetImagesNeedAnimating(true) is called for the document at the end of the swap.

Comment 5

[Mihai Volmer [:mihaivolmer]]

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

Comment 6

[Mihai Volmer [:mihaivolmer]]

Attachment: Forces the document to animate

Comment 7

[Mihai Volmer [:mihaivolmer]]

Attachment: Forces the document to animate

Mike, it seems like you are the last person that has edited this code. Can you review this patch?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8638187 [details] [diff] [review]
Forces the document to animate

Review of attachment 8638187 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Mihai - I'm not 100% sure I'm the right person to review this, but I'll give it a shot. :)

So upon reflection, I actually don't think this should go here. FirePageShowEvent should probably just do what it's describing in the name - extra side effects (like flipping the image animation bool) seem outside the scope of that method.

I think what might make more sense is to flip the bool inside nsFrameLoader::SwapWithOtherLoader once the swap is complete, right after we fire the last pageshow events.

Also, does this bug still persist with e10s with this patch? If so, you'll probably want to flip the bool in TabChild::RecvSwappedWithOtherRemoteLoader after the last pageshow event fires in there.

::: dom/base/nsContentUtils.cpp
@@ +7878,5 @@
>    nsCOMPtr<nsIDocument> doc = aItem->GetDocument();
>    NS_ASSERTION(doc, "What happened here?");
>    if (doc->IsShowing() == aFireIfShowing) {
>      doc->OnPageShow(true, aChromeEventHandler);
> +  } else if (aFireIfShowing == true) {

Instead of == true, might as well just:

else if (aFireIfShowing) {
  // ...
}

Comment 9

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

Mihai, could you revisit this when you get a chance, now that Mike has provided some feedback?

Comment 10

[Mihai Volmer [:mihaivolmer]]

Attachment: Forces the document to animate

Hey Mike. Thank you for reviewing!

I have modified the patch as you specified. Upon reading your review, I
realised it made more sense flipping the bool in the caller-methods and not
in the FirePageShowEvent method.

Also, I have tested both the non-multi-process and the e10s firefox 
and the bug is gone with both the old patch and this one.

Comment 11

[Mihai Volmer [:mihaivolmer]]

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

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8644628 [details] [diff] [review]
Forces the document to animate

Review of attachment 8644628 [details] [diff] [review]:
-----------------------------------------------------------------

Wait a second. What's happening here?

It looks like nsDocument is supposed to call SetImagesNeedAnimating(true) in its OnPageShow handler if the aPersisted argument is set to true.

So I guess the page isn't showing by the time we call nsContentUtils::FirePageShowEvent in nsFrameLoader, which means we don't call OnPageShow? Can you confirm that?

I... suspect we might want smaug to look at this, actually. I think I'm confused about what state this document should be in at the end of a swap.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I think you just need to use the flag ehsan adds in Bug 1191491 and not call
SetImagesNeedAnimating(false); if we're doing swap.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8644628 [details] [diff] [review]
Forces the document to animate

Ah yes, InFrameSwap sounds very useful here.

Comment 15

[Mihai Volmer [:mihaivolmer]]

Attachment: The document should not stop animations when it is being swapped

As Mike suspected, the OnPageShow() method was not called after the OnPageShow() when the bug occured.
I have added a check for InFrameSwap() in the OnPageHide() method.
Also, I have made some tests and mAnimationConsumers maintains its value when the page is swapped.

Comment 16

[Mihai Volmer [:mihaivolmer]]

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

Comment 17

[Mihai Volmer [:mihaivolmer]]

Attachment: The document should not stop animations when it is being swapped

Aparently I messed up with pointers in the first patch.
I just added a safety check.

Comment 18

[Mihai Volmer [:mihaivolmer]]

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

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8646736 [details] [diff] [review]
The document should not stop animations when it is being swapped

>+  // We do not stop the animations (bug 1024343)
>+  // when the page is refreshing while being dragged out
>+  nsRefPtr<nsDocShell> docShell = (nsDocShell*)static_cast<nsIDocShell*>(GetContainer());
nsDocShell* docShell = mDocumentContainer.get();

with that, r+

Comment 20

[Mihai Volmer [:mihaivolmer]]

Attachment: The document should not stop animations when it is being swapped

Thank you for the reviews! I have made the change you have requested.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

So is this waiting on anything before landing? It looks like it's got r+...

Comment 22

[Mihai Volmer [:mihaivolmer]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #21)
> So is this waiting on anything before landing? It looks like it's got r+...

It's not waiting on anything and I think it is ready to land. I wanted to ping Olli about this,  but it slipped my mind.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Add checkin-needed keyword, and someone will land this :)

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62dbe8652b14

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62dbe8652b14

Comment 26

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on Firefox nightly version 33.0a1 according to (2014-06-12) 

It's fixed and verified on Firefox Beta
Build ID 	20151112144305
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS-32bitWindows 8.1

Comment 27

[Josh Matthews [:jdm]]

Thanks for verifying that!