Closed Bug 691792 Opened 13 years ago Closed 13 years ago

Change of src attribute for animated gifs no longer works as expected

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox10 - affected

People

(Reporter: jwir3, Assigned: jwir3)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Change of source test case demonstrating issue with bug 666446
6.06 KB, text/html
Details
Patchv2 [r-dholbert]
17.01 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
example demonstrating animation getting stuck
41.75 KB, text/plain
Details 
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.
Assignee: nobody → sjohnson
Maybe you can fix bug 629411 as part of this, so we can catch it next time :)
(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.
Attached patch Patchv1 (obsolete) — — Splinter Review 
Also pushed to try, try-chooser should automatically post here about that when it's done building.
Attachment #564656 - Flags: review?(dholbert)
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 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 ;)
(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 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.
Attachment #564656 - Flags: review?(dholbert) → review+
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
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.
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
Keywords: checkin-needed
(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.
Keywords: checkin-needed
Dao, thanks for your comments. I have fixed the issues you have described.
Attachment #564656 - Attachment is obsolete: true
Attachment #565237 - Flags: review+
Attachment #565237 - Flags: checkin?(dao)
Keywords: checkin-needed
Attachment #565237 - Flags: checkin?(dao) → checkin?
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
Keywords: checkin-needed
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
}
Status: NEW → ASSIGNED
Attachment #565237 - Flags: checkin?
(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.
(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 :-)
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.)
Flags: in-testsuite?
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.
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.
Attachment #613206 - Attachment mime type: text/plain → text/html
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.
Attachment #613206 - Attachment is obsolete: true
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!
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".)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: