Last Comment Bug 691792 - Change of src attribute for animated gifs no longer works as expected
: Change of src attribute for animated gifs no longer works as expected
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
Depends on:
Blocks: 666446
  Show dependency treegraph
 
Reported: 2011-10-04 09:08 PDT by Scott Johnson (:jwir3)
Modified: 2012-04-08 17:27 PDT (History)
9 users (show)
dholbert: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected


Attachments
Change of source test case demonstrating issue with bug 666446 (6.06 KB, text/html)
2011-10-04 09:08 PDT, Scott Johnson (:jwir3)
no flags Details
Patchv1 (16.63 KB, patch)
2011-10-04 13:33 PDT, Scott Johnson (:jwir3)
dholbert: review+
Details | Diff | Review
Patchv2 [r-dholbert] (17.01 KB, patch)
2011-10-06 08:31 PDT, Scott Johnson (:jwir3)
jaywir3: review+
Details | Diff | Review
example demonstrating animation getting stuck (41.75 KB, text/html)
2012-04-08 16:53 PDT, panzi
no flags Details
example demonstrating animation getting stuck (41.75 KB, text/plain)
2012-04-08 16:56 PDT, panzi
no flags Details

Description Scott Johnson (:jwir3) 2011-10-04 09:08:17 PDT
Created attachment 564572 [details]
Change of source test case demonstrating issue with bug 666446

After the merge of bug 666446, it appears that some animated gif images don't animate after a src attribute is changed. An example testcase has been added to this bug. 

This appears to be an issue with a small block of code included in the original patches for bug 666446 not being included in the final patches that were landed on 10-03-11.
Comment 1 Joe Drew (not getting mail) 2011-10-04 10:04:42 PDT
Maybe you can fix bug 629411 as part of this, so we can catch it next time :)
Comment 2 Scott Johnson (:jwir3) 2011-10-04 11:13:47 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #1)
> Maybe you can fix bug 629411 as part of this, so we can catch it next time :)

Well, actually I added a test for this as part of the tests I added for bug 666446. It only happens in certain cases, though, because the unit test is passing, but when I use it in an interactive context, it isn't working as expected.
Comment 3 Scott Johnson (:jwir3) 2011-10-04 13:33:56 PDT
Created attachment 564656 [details] [diff] [review]
Patchv1

Also pushed to try, try-chooser should automatically post here about that when it's done building.
Comment 4 Scott Johnson (:jwir3) 2011-10-04 13:34:53 PDT
I should mention regarding the patch - the only thing that is new is the tests. The rest of the code was already pre-reviewed as part of bug 666446, but didn't make it into the final set of patches (probably a merge error on my part).
Comment 5 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-04 14:07:51 PDT
Comment on attachment 564656 [details] [diff] [review]
Patchv1

>+  bool* requestFlag = GetRegisteredFlagForRequest(aRequest);
>+  if (requestFlag) {
>+    nsLayoutUtils::RegisterImageRequest(GetFramePresContext(), aRequest,
>+                                        requestFlag);
>+  }

It looks like nsLayoutUtils::RegisterImageRequest already null-checks requestFlag (as well as checking if its pointed-to-value is true).  So, probably no need to check it here, right?

> NS_IMETHODIMP nsBulletListener::OnStartContainer(imgIRequest *aRequest,
>                                                  imgIContainer *aImage)
> {
>   if (!mFrame)
>-    return NS_ERROR_FAILURE;
>+    return NS_OK;

There aren't any bullets in the live portion of the testcase, AFAICT.  Is this change still necessary to fix this? Is it possible to regression-test whatever's broken in this bullet code?

>+++ b/modules/libpr0n/test/mochitest/test_changeOfSource2.html
>@@ -0,0 +1,48 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=666446
>+-->
>+<head>
>+  <title>Test for Bug 666446 - Change of Source (2nd Version)</title>
[...]
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=666446">
>+Mozilla Bug 666446: lots of animated gifs swamp us with paint events
>+</a>

It's probably better to have these bug links point to this bug, but it doesn't matter supermuch.

>+const FAILURE_TIMEOUT = 15000; // Fail early after 120 seconds (2 minutes)

As noted in IRC, this needs a tweak ;)
Comment 6 Scott Johnson (:jwir3) 2011-10-04 14:42:30 PDT
(In reply to Daniel Holbert [:dholbert] from comment #5)
> 
> It looks like nsLayoutUtils::RegisterImageRequest already null-checks
> requestFlag (as well as checking if its pointed-to-value is true).  So,
> probably no need to check it here, right?

Yes, that's true. I've removed the check.

> There aren't any bullets in the live portion of the testcase, AFAICT.  Is
> this change still necessary to fix this? Is it possible to regression-test
> whatever's broken in this bullet code?

Yes, that is true. This was a piece of code that was changed in the original patch (or was supposed to be changed in the original patch), that didn't make it into the final version.  (See https://bugzilla.mozilla.org/show_bug.cgi?id=666446#c320)

> It's probably better to have these bug links point to this bug, but it
> doesn't matter supermuch.

Fixed.

> As noted in IRC, this needs a tweak ;)

Fixed.

I made these fixes in my local patch repository, and we can land it tomorrow.
Comment 7 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-04 14:50:50 PDT
Comment on attachment 564656 [details] [diff] [review]
Patchv1

(In reply to Scott Johnson (:jwir3) from comment #6)
> (In reply to Daniel Holbert [:dholbert] from comment #5)
> > 
> > It looks like nsLayoutUtils::RegisterImageRequest already null-checks
> > requestFlag (as well as checking if its pointed-to-value is true).  So,
> > probably no need to check it here, right?
> 
> Yes, that's true. I've removed the check.

Sorry - I'd misread the null-check in RegisterImageRequest (I initially thought we'd early-return if requestFlag was null).

Per IRC discussion, I think we want to keep your patch's null-check, and just add an "else" clause with an NS_NOTREACHED or NS_ERROR, since (IIUC) we don't expect to be decoding images other than our current/pending requests.

r=me with that fixed.

> Yes, that is true. This was a piece of code that was changed in the original
> patch (or was supposed to be changed in the original patch), that didn't
> make it into the final version.  (See
> https://bugzilla.mozilla.org/show_bug.cgi?id=666446#c320)

Ok, sounds good.
Comment 8 Mozilla RelEng Bot 2011-10-04 18:21:32 PDT
Try run for cd444c824d8c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cd444c824d8c
Results (out of 171 total builds):
    exception: 1
    success: 148
    warnings: 17
    failure: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sjohnson@mozilla.com-cd444c824d8c
Comment 9 Scott Johnson (:jwir3) 2011-10-05 09:24:38 PDT
The final version of this patch is available at:
http://hg.mozilla.org/users/sjohnson_mozilla.com/patches/file/3f4a88de6d4f/b691792

I am pushing again to try to verify that the problems found in comment 8 were resolved.
Comment 10 Scott Johnson (:jwir3) 2011-10-05 18:14:20 PDT
The oranges for this bug on try push https://tbpl.mozilla.org/?tree=Try&rev=b4191222b830 appear to be random intermittent oranges. So, it looks like this bug should be ready to push to m-i.

Note that the latest version of the patch is at: 
http://hg.mozilla.org/users/sjohnson_mozilla.com/patches/file/3f4a88de6d4f/b691792
Comment 11 Dão Gottwald [:dao] 2011-10-06 02:46:34 PDT
(In reply to Scott Johnson (:jwir3) from comment #10)
> Note that the latest version of the patch is at: 
> http://hg.mozilla.org/users/sjohnson_mozilla.com/patches/file/3f4a88de6d4f/b691792

Is it different from the one attached to this bug? If so, please attach the patch that's supposed to land.

"Fix regression issues introduced by bug 666446" is not a reasonable commit message, please fix this as well.
Comment 12 Scott Johnson (:jwir3) 2011-10-06 08:31:17 PDT
Created attachment 565237 [details] [diff] [review]
Patchv2 [r-dholbert]

Dao, thanks for your comments. I have fixed the issues you have described.
Comment 13 Mozilla RelEng Bot 2011-10-06 12:02:22 PDT
Try run for b4191222b830 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b4191222b830
Results (out of 216 total builds):
    exception: 1
    success: 173
    warnings: 18
    failure: 24
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sjohnson@mozilla.com-b4191222b830
Comment 14 Ed Morley [:emorley] 2011-10-07 03:03:09 PDT
Sorry, no longer applies cleanly to inbound. If it were just a hunk or two I'd try to fix locally, but quite a bit doesn't apply, so I'd likely just end up breaking something:
{
patching file modules/libpr0n/test/mochitest/Makefile.in
Hunk #1 FAILED at 95
1 out of 1 hunks FAILED -- saving rejects to file modules/libpr0n/test/mochitest/Makefile.in.rej
unable to find 'modules/libpr0n/test/mochitest/animationPolling.js' for patching
5 out of 5 hunks FAILED -- saving rejects to file modules/libpr0n/test/mochitest/animationPolling.js.rej
unable to find 'modules/libpr0n/test/mochitest/test_changeOfSource.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file modules/libpr0n/test/mochitest/test_changeOfSource.html.rej
}
Comment 15 Scott Johnson (:jwir3) 2011-10-07 04:26:07 PDT
(In reply to Ed Morley [:edmorley] from comment #14)
> Sorry, no longer applies cleanly to inbound. If it were just a hunk or two
> I'd try to fix locally, but quite a bit doesn't apply, so I'd likely just
> end up breaking something:
> {
> patching file modules/libpr0n/test/mochitest/Makefile.in
> Hunk #1 FAILED at 95
> 1 out of 1 hunks FAILED -- saving rejects to file
> modules/libpr0n/test/mochitest/Makefile.in.rej
> unable to find 'modules/libpr0n/test/mochitest/animationPolling.js' for
> patching
> 5 out of 5 hunks FAILED -- saving rejects to file
> modules/libpr0n/test/mochitest/animationPolling.js.rej
> unable to find 'modules/libpr0n/test/mochitest/test_changeOfSource.html' for
> patching
> 1 out of 1 hunks FAILED -- saving rejects to file
> modules/libpr0n/test/mochitest/test_changeOfSource.html.rej
> }
Bug 666446 was backed out while we sort out regressions, and this depends on those patches. Chances are this will be added into the set of patches that are ultimately pushed up for that bug, so this might be able to be closed.
Comment 16 Ed Morley [:emorley] 2011-10-07 04:32:23 PDT
(In reply to Scott Johnson (:jwir3) from comment #15)
> Bug 666446 was backed out while we sort out regressions

Ah yes, just spotted that in this morning's bugmail :-)
Comment 17 Scott Johnson (:jwir3) 2011-10-25 10:38:19 PDT
This is no longer a problem, as bug 666446 was backed out. When bug 666446 is sent back up, this patch will be included in the patches for bug 666446.
Comment 18 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-25 10:46:43 PDT
Setting "in-testsuite?" flag to remind us to be sure we've got a test for this.  (I think you've already written one, so just set the flag to "+" sometime after that test lands.)
Comment 19 panzi 2012-04-08 16:52:02 PDT
I think this bug reappeared in Firefox 11.0. While the provided example seems to work fine in Firefox 11 I managed to write a test case where it still happens. When you click back and forth between the two buttons in the attachment "pony_gets_stuck.html" the animation gets stuck eventually (at the second or third click). The buttons change the src attribute and the width and height styles.

I used a data:-url in the example just so everything is included in the html file. The same thing occurs when it's a normal url to a gif on a web server.
Comment 20 panzi 2012-04-08 16:53:24 PDT
Created attachment 613206 [details]
example demonstrating animation getting stuck

When you click back and forth between the two buttons in the attachment "pony_gets_stuck.html" the animation gets stuck eventually (at the second or third click). The buttons change the src attribute and the width and height styles.

I used a data:-url in the example just so everything is included in the html file. The same thing occurs when it's a normal url to a gif on a web server.
Comment 21 panzi 2012-04-08 16:56:32 PDT
Created attachment 613207 [details]
example demonstrating animation getting stuck

When you click back and forth between the two buttons in the attachment "pony_gets_stuck.html" the animation gets stuck eventually (at the second or third click). The buttons change the src attribute and the width and height styles.

I used a data:-url in the example just so everything is included in the html file. The same thing occurs when it's a normal url to a gif on a web server.
Comment 22 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-04-08 16:58:58 PDT
panzi: Thanks for the report!

For the record, I can't actually reproduce the problem you describe (i tried Firefox 11 & our Nightly builds, with lots of clicking back and forth, and it still animated).

However, regardless of that -- we should track the issue you describe in a new bug, rather than building off of an existing bug report.

So -- if you could file a new bug for this, that would be great.  Here's the URL with product/component already filled out:
  https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=ImageLib

Thanks!
Comment 23 panzi 2012-04-08 17:27:40 PDT
Ok, I wrote a report. See bug 743598

FYI: I'm not the only one that has this bug. Actually some user of a script of mine reported it to me. I usually use Chrome and didn't notice this myself, but I was able to reproduce it. (It's not often that I find a bug that someone else also has. Usually it's "cant reproduce, must be your setup".)

Note You need to log in before you can comment on or make changes to this bug.