878037 - Crash on print if list-style-image (or :after|:before content) is animated gif

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Francesco]

Attachment: testcase.htm

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

Visit or create a page where a list-style-image is an animated gif.
Print or print preview.

It also happens if :after or :before content is an animated gif; see comments in attached example. Does not depend on the particular gif linked in the attachment (I picked it just to build a test case).


Actual results:

Firefox crashes.


Expected results:

Firefox should not crash.

Comment 1

[Scoobidiver (away)]

Congratulations! You find a testcase for a top crasher.

Here is the crash report: bp-399eed5e-d95e-4b3b-834a-872a52130531.

Comment 2

[Joe Drew (not getting mail)]

oh my goodness!!!

Comment 3

[Alice0775 White]

Rehression window
Good:
http://hg.mozilla.org/mozilla-central/rev/2360c3c46aca
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130204 Firefox/21.0 ID:20130204162621
Crash:
http://hg.mozilla.org/mozilla-central/rev/8f4ec9046b03
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205021221
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2360c3c46aca&tochange=8f4ec9046b03

Regressed by: 
8f4ec9046b03	Olli Pettay — Bug 836875 - Background is no longer painted in Print output/Print preview, r=roc

Comment 4

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

Um, so we're crashing on purpose.

I don't see anything wrong with bug 836875. Does imglib have some odd expectations, or can it
not handle static clones well or what?

Comment 5

[Alice0775 White]

OK

appear the crash ----- by Bug 801061
hide the crash   ----- by Bug 817342
re-appear the crash -- by Bug 836875


#1 appear the crash window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/bb2f453b7c0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121214 Firefox/20.0 ID:20121215061721
Crash:
http://hg.mozilla.org/mozilla-central/rev/c8a1314aa449
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121215 Firefox/20.0 ID:20121215131821
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e3e594837a30&tochange=75af94f85a98

#1 appear the crash window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/57bbbda0dedd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121215 Firefox/20.0 ID:20121215074621
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1d81cc19ff3f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121215 Firefox/20.0 ID:20121215090824
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=57bbbda0dedd&tochange=1d81cc19ff3f
Suspected
82077de3f9bc	Jeff Muizelaar — Bug 801061. Switch from imgIRequest to imgRequestProxy. r=joe This doesn't switch all of the users yet, but is a step in the right direction.


#2hide the crash window(m-c)
Crash:
http://hg.mozilla.org/mozilla-central/rev/a2c46e2c7df1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130110 Firefox/21.0 ID:20130110010011
Hiding the crash:
http://hg.mozilla.org/mozilla-central/rev/5a74a94f6d44
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130110 Firefox/21.0 ID:20130110024509
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a2c46e2c7df1&tochange=5a74a94f6d44

Hiding the crash by:
5a74a94f6d44	Olli Pettay — Bug 817342, delay script runners when making static clones, r=roc


#3re-appear the crash window
Good:
http://hg.mozilla.org/mozilla-central/rev/2360c3c46aca
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130204 Firefox/21.0 ID:20130204162621
Re-appears the crash:
http://hg.mozilla.org/mozilla-central/rev/8f4ec9046b03
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205021221
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2360c3c46aca&tochange=8f4ec9046b03

Re-appears the crash by: 
8f4ec9046b03	Olli Pettay — Bug 836875 - Background is no longer painted in Print output/Print preview, r=roc

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Um, so we're crashing on purpose.
> 
> I don't see anything wrong with bug 836875. Does imglib have some odd
> expectations, or can it
> not handle static clones well or what?

The purposeful crashes were added in bug 853774 recently to try to get more information on why a top crash was happening where we did not have any way to reproduce.

Comment 7

[Joe Drew (not getting mail)]

Attachment: revert explicit crashes from bug 853774

This is simply an hg revert of the debugging patch from bug 853774.

Comment 8

[Joe Drew (not getting mail)]

Attachment: Make both imgRequestProxy Clone overrides virtual

This bug was caused by the version of Clone() that callers chose being non-virtual, despite their concrete type being imgRequestProxyStatic. Making the important Clone virtual should fix this bug (still building).

Comment 9

[Joe Drew (not getting mail)]

Olli, is there an easy way to add a printing test? I wrote a mochitest for this, but only realized it couldn't possibly work (as it relied upon not calling virtual functions) after having done so.

Comment 10

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

we have some (way too few!) print preview tests. Is such enough?
http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/printpreview_helper.xul
for example

Comment 11

[Joe Drew (not getting mail)]

Attachment: test static clone

This passes before and after, but it's reasonable to have a test for it!

Comment 12

[Joe Drew (not getting mail)]

Attachment: print preview test

This is a print preview test specifically for this bug. Unfortunately print preview tests don't run on OS X so I had to throw it to try: https://tbpl.mozilla.org/?tree=Try&rev=4d15d34917bc

Comment 13

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

I should perhaps review after try results :)

Comment 14

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

Comment on attachment 757575 [details] [diff] [review]
Make both imgRequestProxy Clone overrides virtual

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

Nice. It ended up being something so simple.

Comment 15

[Joe Drew (not getting mail)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=4d15d34917bc

This was actually supposed to fail - it didn't have the patch! :(

Is setting .innerHTML synchronous? That could certainly cause a problem if not!

Comment 16

[Joe Drew (not getting mail)]

Anyways, here's a try that should succeed (it has the tests and the patches): https://tbpl.mozilla.org/?tree=Try&rev=306baa84b2e3

Comment 17

[Joe Drew (not getting mail)]

what hath jrmuizel wrought

../../dist/include/imgRequestProxy.h:50:552: error: 'virtual nsresult imgRequestProxy::Clone(imgINotificationObserver*, imgIRequest**)' was hidden [-Werror=overloaded-virtual]
../../dist/include/imgRequestProxy.h:234:20: error:   by 'virtual nsresult imgRequestProxyStatic::Clone(imgINotificationObserver*, imgRequestProxy**)' [-Werror=overloaded-virtual]

Comment 18

[Alex Keybl [:akeybl]]

Low risk enough to land directly to m-a/m-b today in preparation for b4?

Comment 19

[Joe Drew (not getting mail)]

(In reply to Alex Keybl [:akeybl] from comment #18)
> Low risk enough to land directly to m-a/m-b today in preparation for b4?

It's not zero-risk since we're adding a virtual function to the vtable, but after some discussion we think it should be safe.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 757575 [details] [diff] [review]
Make both imgRequestProxy Clone overrides virtual

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It's complicated (see comment 5). Basically, Bug 801061.
User impact if declined: Crashes when printing certain documents.
Testing completed (on m-c, etc.): tested locally and on try
Risk to taking this patch (and alternatives if risky): We export imgRequestProxy.h now, which means that it's possible that external people can look at it, and this changes the binary size of imgRequestProxy. But we don't export the *symbols* for imgRequestProxy, so nobody should have ever done that since it wouldn't link. It's not zero-risk, but it's close to zero.
String or IDL/UUID changes made by this patch: none, but see above

Comment 21

[Joe Drew (not getting mail)]

I'm having trouble getting the tests be useful, but I've pushed the code changes:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d11ed612690
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd83c475006

Comment 22

[Joe Drew (not getting mail)]

Attachment: print preview crashtest

This test is an effective crashtest for this bug.

Try that should crash (without this bug's patches): https://tbpl.mozilla.org/?tree=Try&rev=ada43f56b7d7
Try that should work (with this bug's patches): https://tbpl.mozilla.org/?tree=Try&rev=9d73fcac2f19

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 757575 [details] [diff] [review]
Make both imgRequestProxy Clone overrides virtual

Approving for all branches because we'd like to take this in b4.

Comment 24

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/089956e907ed
https://hg.mozilla.org/releases/mozilla-beta/rev/2959344b8ad6

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d11ed612690
https://hg.mozilla.org/mozilla-central/rev/4dd83c475006

Comment 26

[Joe Drew (not getting mail)]

Naturally I didn't actually push the patch I uploaded.

Should pass: https://tbpl.mozilla.org/?tree=Try&rev=7621bcc84ad8
Should fail: https://tbpl.mozilla.org/?tree=Try&rev=d899ce507d70

Comment 28

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

Comment on attachment 758114 [details] [diff] [review]
print preview crashtest

># HG changeset patch
># Parent 7e3a4ebcf067f387ddceabb22c2b4bf440315511
>
>diff --git a/layout/base/tests/chrome/Makefile.in b/layout/base/tests/chrome/Makefile.in
>--- a/layout/base/tests/chrome/Makefile.in
>+++ b/layout/base/tests/chrome/Makefile.in
>@@ -30,16 +30,17 @@ MOCHITEST_CHROME_FILES = \
> 	test_default_background.xul \
> 	     default_background_window.xul \
> 	test_no_clip_iframe.xul \
> 	     no_clip_iframe_window.xul \
> 	     no_clip_iframe_subdoc.html \
> 	test_prerendered_transforms.html \
> 	test_printpreview.xul \
> 	     printpreview_helper.xul \
>+	     animated.gif \
> 	test_printpreview_bug396024.xul \
> 	     printpreview_bug396024_helper.xul \
> 	test_printpreview_bug482976.xul \
> 	     printpreview_bug482976_helper.xul \
> 	test_scrolling_repaints.html \
> 	test_transformed_scrolling_repaints.html \
> 	test_transformed_scrolling_repaints_2.html \
> 	test_transformed_scrolling_repaints_3.html \
>diff --git a/layout/base/tests/chrome/animated.gif b/layout/base/tests/chrome/animated.gif
>new file mode 100644
>index 0000000000000000000000000000000000000000..b2895487bd5df3407a207bd50acd668fff0fd032
>GIT binary patch
>literal 527
>zc${<hbhEHbOkqf2XkcXc&%p5i|9{1wES!uCj0`#qK;R9OVPM+R)4%fcTmHp!w%qF8
>zd~eTh{<cRR)1GxMdv$8tJMQD3e6D}%eg5m*_ka96T<u2|eeCejny%RJnX}hxdB}^E
>zU1yWl-&!~QHE;OAN1Lvu_}QP#GTi;w_3-KUmD}><Pk)ZJiGTIIJWOeYXaq+=#0rfQ
>zKc1gppQ)bdZkf*6eskH%V>6#+vUzQ9w@`iM79~|Gbxg{AMb<2-*H3PlduL~NNUyFw
>zzWUW}))=id$6MyC*>mR3n?F3Vy0*TtxwXA>bAuQdP&{M;_Rt0*J@lS|1MDFcr%7kj
>zieL74ExeR9DeJ73{H=EaueUZG4%V}@wllx{YV}Up)Wf}>ewt;(@Be(l>i&H840Q{4
>zj`SOCE0$Fp^LQq~_EPnx>&n#5ZO7f0@2tH3;O2Ent_DprsjR4_QEl$OPJW0wBGvxN
>z^L5MW=UUNRU$|v-Y<yyJYI?vP`V)ZW32Cq=0`Pc(<p|bTIkOBLD_2&1?K+#a{m#1Y
>U*|nX=pKSWs<G1?yY9<D20B+X@R{#J2
>
>diff --git a/layout/base/tests/chrome/printpreview_helper.xul b/layout/base/tests/chrome/printpreview_helper.xul
>--- a/layout/base/tests/chrome/printpreview_helper.xul
>+++ b/layout/base/tests/chrome/printpreview_helper.xul
>@@ -239,16 +239,33 @@ function runTest4end() {
> 
> // This is a crash test for bug 595337
> function runTest5() {
>   window.frames[0].document.body.innerHTML =
>     '<iframe style="position: fixed; visibility: hidden; bottom: 10em;"></iframe>' +
>     '<input contenteditable="true" style="display: table; page-break-before: left; width: 10000px;">';
>   printpreview();
>   exitprintpreview();
>+
>+  setTimeout(runTest6, 0);
>+}
>+
>+// Crash test for bug 878037
>+function runTest6() {
>+  window.frames[0].document.body.innerHTML =
>+    '<style> li { list-style-image: url("animated.gif"); } </style>' +
>+    '<li>Firefox will crash if you try and print this page</li>';
>+
>+  setTimeout(runTest6end, 500);
>+}
>+
>+function runTest6end() {
>+  printpreview();
>+  exitprintpreview();
>+
>   finish();
> }
> 
> ]]></script>
> <table style="border: 1px solid black;" xmlns="http://www.w3.org/1999/xhtml">
> <tr><th>Print preview canvas 1</th><th>Print preview canvas 2</th></tr>
> <tr>
> <td><canvas height="300" width="300"></canvas></td>

Comment 29

[Cornel Ionce [:noni]]

Verified as fixed for FF 22 beta 4 on Windows 7, Ubuntu 12.04 and Mac OSX 10.6 with the attached testcase.


Build ID: 20130605070403

Comment 30

[Joe Drew (not getting mail)]

Pushed the tests:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65c588d76efa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34656a2a1234

Comment 31

[Simona Badau, Desktop Test Engineering]

Verified as fixed on Firefox 23 beta 3:

Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130703181823