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.

(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

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

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

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) { // ... }

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

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

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 on attachment 8644628 [details] [diff] [review] Forces the document to animate Ah yes, InFrameSwap sounds very useful here.

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.

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

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+

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

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

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

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

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

Thanks for verifying that!