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

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: tashmore, Assigned: mihaivolmer, Mentored)

Tracking

32 Branch
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 verified)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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
Status: UNCONFIRMED → NEW
Component: Untriaged → ImageLib
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Assignee: nobody → mvolmer
Mentor: seth
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
(Assignee)

Comment 4

4 years ago
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.
Attachment #8636973 - Flags: review?(seth)
(Assignee)

Comment 6

4 years ago
(Assignee)

Updated

4 years ago
Attachment #8636973 - Attachment is obsolete: true
Attachment #8636973 - Flags: review?(seth)
(Assignee)

Comment 7

4 years ago
Mike, it seems like you are the last person that has edited this code. Can you review this patch?
Attachment #8638187 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8638186 - Attachment is obsolete: true
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) {
  // ...
}
Attachment #8638187 - Flags: review?(mconley) → review-
Mihai, could you revisit this when you get a chance, now that Mike has provided some feedback?
Flags: needinfo?(mvolmer)
(Assignee)

Comment 10

4 years ago
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.
Attachment #8644628 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8638187 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(mvolmer)
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.
Attachment #8644628 - Flags: review?(bugs)
I think you just need to use the flag ehsan adds in Bug 1191491 and not call
SetImagesNeedAnimating(false); if we're doing swap.
Attachment #8644628 - Flags: review?(bugs) → review-
Comment on attachment 8644628 [details] [diff] [review]
Forces the document to animate

Ah yes, InFrameSwap sounds very useful here.
Attachment #8644628 - Flags: review?(mconley)
(Assignee)

Comment 15

4 years ago
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.
Attachment #8646680 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8644628 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Aparently I messed up with pointers in the first patch.
I just added a safety check.
(Assignee)

Updated

4 years ago
Attachment #8646680 - Attachment is obsolete: true
Attachment #8646680 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8646736 - Flags: review?(bugs)
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+
Attachment #8646736 - Flags: review?(bugs) → review+
(Assignee)

Comment 20

4 years ago
Thank you for the reviews! I have made the change you have requested.
(Assignee)

Updated

4 years ago
Attachment #8646736 - Attachment is obsolete: true
So is this waiting on anything before landing? It looks like it's got r+...
Flags: needinfo?(mvolmer)
(Assignee)

Comment 22

4 years ago
(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.
Flags: needinfo?(mvolmer)
Add checkin-needed keyword, and someone will land this :)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62dbe8652b14
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
QA Whiteboard: [testday-20151113]
Thanks for verifying that!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.