Last Comment Bug 730066 - Crash at mozilla::layers::ImageContainer::GetCurrentSize on printing http://www.mozilla.org/projects/firefox/prerelease.html
: Crash at mozilla::layers::ImageContainer::GetCurrentSize on printing http://w...
Status: RESOLVED FIXED
: crash, regression, reproducible, testcase
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 13 Branch
: All All
: -- critical (vote)
: mozilla14
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on: 778642
Blocks: 715785
  Show dependency treegraph
 
Reported: 2012-02-23 12:24 PST by Rishab Arora [:spacetime]
Modified: 2012-08-01 07:16 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected


Attachments
gdb.txt (15.04 KB, text/plain)
2012-02-23 12:24 PST, Rishab Arora [:spacetime]
no flags Details
Another, more informative gdb stack trace (OS X) (41.12 KB, text/plain)
2012-03-19 11:24 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details
Proof of concept patch (1.10 KB, patch)
2012-03-19 12:39 PDT, Steven Michaud [:smichaud] (Retired)
roc: review+
Details | Diff | Splinter Review

Description Rishab Arora [:spacetime] 2012-02-23 12:24:56 PST
Created attachment 600136 [details]
gdb.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11

Steps to reproduce:

1) Open www.mozilla.org/projects/firefox/prerelease.html
2) Close the browser (have Show windows and tabs from last session enabled) OR kill the firefox process
3) Recover the tabs (if killed), or the tab will be there already
4) Click on File->Print Preview

Nightly 13.0a1 (2012-02-24)
changeset:   87509:88aab8279621


Actual results:

Crash. With Segmentation fault on the terminal.
Crash window does not appear. gdb information (including backtrace) is in gdb.txt


Expected results:

Print Preview , no crashes
Comment 1 Alice0775 White 2012-02-23 13:12:12 PST
Confirmed
http://hg.mozilla.org/mozilla-central/rev/5e756e59a794
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120223 Firefox/13.0a1 ID:20120223031236

bp-ad045a11-30d5-4d85-b5eb-9739c2120223
Comment 2 Alice0775 White 2012-02-23 13:50:20 PST
Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/1cdef0321abd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120202 Firefox/13.0a1 ID:20120202010526
Crashes:
http://hg.mozilla.org/mozilla-central/rev/005980552224
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120202 Firefox/13.0a1 ID:20120202022426
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1cdef0321abd&tochange=005980552224


http://hg.mozilla.org/integration/mozilla-inbound/rev/b2f1b368e2f2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120131 Firefox/13.0a1 ID:20120131180632
Crashes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/67b0e13d7a62
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120131 Firefox/13.0a1 ID:20120131181932
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2f1b368e2f2&tochange=67b0e13d7a62

Regressed by:
67b0e13d7a62	Bas Schouten — Bug 715785: Make ImageContainers independent of LayerManagers. r=roc
Comment 3 Virgil Dicu [:virgil] [QA] 2012-03-15 03:24:46 PDT
(In reply to Rishab Arora from comment #0)
> Steps to reproduce:
> 
> 1) Open www.mozilla.org/projects/firefox/prerelease.html
> 2) Close the browser (have Show windows and tabs from last session enabled)
> OR kill the firefox process
> 3) Recover the tabs (if killed), or the tab will be there already
> 4) Click on File->Print Preview

I can reproduce this with simply selecting Ctrl+P and print on the page from 1) on all platforms.
Comment 4 Steven Michaud [:smichaud] (Retired) 2012-03-19 11:24:37 PDT
Created attachment 607226 [details]
Another, more informative gdb stack trace (OS X)

I've confirmed Virgil's STR from comment #3 on OS X 10.6.8.

And I've confirmed Alice's report from comment #2 that these crashes were triggered by Bas Schouten's patch for bug 715785 (https://hg.mozilla.org/mozilla-central/rev/67b0e13d7a62).

Also, here's a more informative gdb stack trace, made on OS X 10.6.8 with a build made from today's mozilla-central code.
Comment 5 Steven Michaud [:smichaud] (Retired) 2012-03-19 11:35:54 PDT
Apparently mozilla::layers::ImageContainer::GetCurrentSize() is getting called on a deleted ImageContainer object.
Comment 6 Steven Michaud [:smichaud] (Retired) 2012-03-19 11:42:11 PDT
> Apparently mozilla::layers::ImageContainer::GetCurrentSize() is
> getting called on a deleted ImageContainer object.

Or maybe it's just a NULL ImageContainer object.
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-03-19 12:39:02 PDT
Created attachment 607262 [details] [diff] [review]
Proof of concept patch

Comment #6 turns out to be right.

Here's a proof of concept patch that fixes these crashes in my tests on OS X 10.6.8.  I've started tryserver builds on all platforms, which should be available in a few hours.  Please test with them once they become available.

This patch isn't the best fix for these crashes -- at least not by itself.  Bas's patch for bug 715785 makes an assumption that turns out to be incorrect (that nsHTMLVideoElement::GetImageContainer() always returns a usable object), and there are other parts of the tree that would also need to be changed.

Others know this code much better than I do ... like Bas, to whom I'm going to assign this bug.
Comment 8 Steven Michaud [:smichaud] (Retired) 2012-03-19 12:40:48 PDT
Bas, you know this code much better than I do :-)

Please reassign to someone else if that's appropriate.
Comment 9 Steven Michaud [:smichaud] (Retired) 2012-03-19 18:31:59 PDT
Here are the tryserver builds made with my patch from comment #7.  Please try them out and report your results here.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-76aee1ac1c4d/
Comment 10 Marcia Knous [:marcia - use ni] 2012-03-22 16:37:15 PDT
Steven: I tried the build in Comment 9 on Mac 10.7.4 and Windows 7 x64 and can confirm that I do not get the crash following the STR in Comment 3.

In both cases I tested with the latest nightly on those systems and was able to generate a crash, so from what I can see the tryserver build does the trick.

(In reply to Steven Michaud from comment #9)
> Here are the tryserver builds made with my patch from comment #7.  Please
> try them out and report your results here.
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> 76aee1ac1c4d/
Comment 11 Virgil Dicu [:virgil] [QA] 2012-03-26 02:22:13 PDT
Also confirming on Linux. No crash with the Try Server build.

(In reply to Steven Michaud from comment #9)
> Here are the tryserver builds made with my patch from comment #7.  Please
> try them out and report your results here.
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> 76aee1ac1c4d/
Comment 12 Steven Michaud [:smichaud] (Retired) 2012-04-06 13:27:29 PDT
Comment on attachment 607262 [details] [diff] [review]
Proof of concept patch

Apparently Bas doesn't have time to deal with this bug.  So I've taken
another look the the code myself, to see if I can do better than my
proof of concept patch in comment #7.

I can't.  Though my patch isn't what Bas originally intended, before
he removed the null check from his original patch (see bug 715785
comment #1), I still don't see anything wrong with it.

So Roc, I'll ask you to review.
Comment 13 Jesse Ruderman 2012-04-07 02:03:01 PDT
Printing layout/reftests/ogg-video/black140x100.ogv to PDF hits the same crash, so it should be easy to add a test.
Comment 14 Steven Michaud [:smichaud] (Retired) 2012-04-09 12:19:11 PDT
Comment on attachment 607262 [details] [diff] [review]
Proof of concept patch

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/736f897f8a0e
Comment 15 Steven Michaud [:smichaud] (Retired) 2012-04-09 12:21:23 PDT
(In reply to comment #13)

On OS X the print and print preview dialogs are both native, so an automated test wouldn't work -- the dialog would never get closed.  (In fact the print-preview dialog is spawned from the print dialog.)
Comment 16 Jesse Ruderman 2012-04-09 13:40:17 PDT
Yeah, and bug 675709.

Does the crash reproduce with reftest-print, or is that too different from actual printing?

Could you add a test and skip it on Mac?

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