Closed Bug 878037 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: ImageLib, defect, critical)

20 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- affected
firefox22 + verified
firefox23 + verified
firefox24 + fixed

People

(Reporter: vertova, Assigned: joe)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(5 files, 1 obsolete file)

Attached file 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.
Congratulations! You find a testcase for a top crasher.

Here is the crash report: bp-399eed5e-d95e-4b3b-834a-872a52130531.
Blocks: 853774
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ imgRequest::GetStatusTracker() ]
Component: Untriaged → ImageLib
Ever confirmed: true
Keywords: crash, testcase
OS: Windows XP → All
Product: Firefox → Core
Hardware: x86 → All
Version: 21 Branch → Trunk
oh my goodness!!!
Attachment #756532 - Attachment mime type: text/plain → text/html
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
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?
Version: Trunk → 20 Branch
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
Blocks: 801061
(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.
Assignee: nobody → joe
This is simply an hg revert of the debugging patch from bug 853774.
Attachment #757571 - Flags: review?(seth)
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).
Attachment #757575 - Flags: review?(seth)
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.
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
This passes before and after, but it's reasonable to have a test for it!
Attachment #757665 - Flags: review?(seth)
Attached patch print preview test (obsolete) — Splinter Review
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
Attachment #757666 - Flags: review?(bugs)
I should perhaps review after try results :)
Attachment #757571 - Flags: review?(seth) → review+
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.
Attachment #757575 - Flags: review?(seth) → review+
Attachment #757665 - Flags: review?(seth) → review+
(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!
Anyways, here's a try that should succeed (it has the tests and the patches): https://tbpl.mozilla.org/?tree=Try&rev=306baa84b2e3
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]
Low risk enough to land directly to m-a/m-b today in preparation for b4?
Flags: needinfo?(joe)
(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.
Flags: needinfo?(joe)
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
Attachment #757575 - Flags: approval-mozilla-beta?
Attachment #757575 - Flags: approval-mozilla-aurora?
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
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
Attachment #757666 - Attachment is obsolete: true
Attachment #757666 - Flags: review?(bugs)
Attachment #758114 - Flags: review?(bugs)
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.
Attachment #757575 - Flags: approval-mozilla-beta?
Attachment #757575 - Flags: approval-mozilla-beta+
Attachment #757575 - Flags: approval-mozilla-aurora?
Attachment #757575 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/9d11ed612690
https://hg.mozilla.org/mozilla-central/rev/4dd83c475006
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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
Duplicate of this bug: 853774
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>
Attachment #758114 - Flags: review?(bugs) → review+
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
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
You need to log in before you can comment on or make changes to this bug.