411369 - Crash while trying to print [@ @0x0 @0x1000 _cairo_meta_surface_create] [@ _moz_cairo_surface_finish ]

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Martijn Wargers (dead)]

With every page, this crash happens for me here on current trunk build on Vista.

http://crash-stats.mozilla.com/report/index/4a0b1611-be42-11dc-aa45-001a4bd43ef6

On branch I get a "Printing failed when starting the document." error instead, which is at least better than crashing.

I haven't looked at when this regressed.

Comment 1

[Martijn Wargers (dead)]

It also happens when printing to a file.
I don't crash with my debug build, but that build is a month old (20071204).
Let's see what happens after I have updated my debug build.

Comment 2

[Martijn Wargers (dead)]

I just installed the XPS Document Writer driver on my windowsXP machine, but I don't crash there.

Comment 3

[Martijn Wargers (dead)]

Hrm, I'm actually crashing with everything while trying to print with Firefox trunk on my Vista machine.

Comment 4

[Martijn Wargers (dead)]

And now it's also happening on my windowsXP machine. There is clearly something wacky going on.
Can other persons besides me reproduce this?

Comment 5

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008010912 Minefield/3.0b3pre ID:2008010912

crash here too when trying to print with pdfcreator
http://sourceforge.net/projects/pdfcreator

I'm sure this worked a few weeks ago

Comment 6

[Peter van der Woude [:Peter6]]

there you go
regressionwindow:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1199823480&maxdate=1199831099&cvsroot=%2Fcvsroot

Comment 7

[Martijn Wargers (dead)]

Thanks for that regression window, CC-ing Neil, although it seems weird to me those check-ins would have caused this.

Comment 8

[Peter van der Woude [:Peter6]]

(In reply to comment #7)
> Thanks for that regression window, CC-ing Neil, although it seems weird to me
> those check-ins would have caused this.
> 
I doublechecked , using the build before and after the regressionwindow
It's Bug 376578 or Bug 409242

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

FWIW I am also crashing on Vista trying to print anything but a plain text page, like a bug here. 

I have no idea what XPS DocWriter is... mostly just a plain VistaHP install here as I've been a bit gun-shy to go off tweaking and adding stuff willy-nilly.

Latest hourly:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008010915 Minefield/3.0b3pre Firefox/3.0 ID:2008010915

Comment 10

[Steffen Wilberg]

I can't print at all, crashing every time, using any printer, on Windows XP.
I can confirm the regression window: Minefield 2008010805 works, but 2008010813 is broken.

Comment 11

[Martijn Wargers (dead)]

Sounds more like a blocker than.
The XPS DocWriter stuff is more or less MS PDF. It's just a way of printing to a file instead of wasting tons of paper.

Comment 12

[Brian Polidoro]

Here's a crash I got which is in _cairo_meta_surface_create
bp-4e0f7ebd-bf8b-11dc-bf04-001a4bd46e84

Also if you widen where the bonsai query searches:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1199823480&maxdate=1199831099&cvsroot=%2Fcvsroot

It looks like Bug 361343 is the culprit due to the new optimization.  

Comment 13

[Robert Sayre]

Seems like the first order of business is running this under Purify. I can do this tomorrow, but maybe someone can get to it sooner.

Comment 14

[Peter van der Woude [:Peter6]]

note: the fix from Bug 408538 didn't fix this one

Comment 16

[Jean-Michel Reghem]

This page doesn't crash when trying to print the page: 
http://fr.wikipedia.org/wiki/P%C3%A9trusse

This one crash: http://fr.wikipedia.org/wiki/Alzette

call stack: bp-29999611-c04e-11dc-9e52-001a4bd43e5c

Comment 17

[Samuel Sidler (old account; do not CC)]

This is a new topcrash, according to Socorro.

http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0b3pre&range_value=2&signature=%400x1000

Comment 19

[Samuel Sidler (old account; do not CC)]

This is a critical regression, a blocker for smoketests, a problem for dogfooding on Windows Vista, and I consider it a Zero Tolerance New Crash.

This bug should be blocking and fixed ASAP or the patches in the regression window need to be backed out.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No crash with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011104 Minefield/3.0b3pre ID:2008011104

Seems to be a Windows only bug.

Comment 22

[Mike Schroepfer]

Be nice to get this fixed for B3 given the reproducibility and the number of dups..

Comment 23

[Robert Sayre]

Attachment: gavin's purify session

Comment 24

[Robert Sayre]

While linking, I get this:

c:\wpo_firefox\mozilla\jpeg\jidctint.c(918) : 
warning C4731: 'ippiDCTQuantInv8x8LS_JPEG_16s8u_C1R' : frame pointer register 'ebx' modified by inline assembly code

Comment 25

[Robert Sayre]

Attachment: wikipedia image 1

Comment 26

[Robert Sayre]

Attachment: wikipedia image 2

Comment 27

[Robert Sayre]

Attachment: test page

Comment 28

[Robert Sayre]

Attachment: test image 1

Comment 29

[Robert Sayre]

Comment on attachment 296882 [details]
test image 1

Trying to print this image will crash you.

Comment 30

[Robert Sayre]

ruled out bug 406580.

Comment 31

[Robert Sayre]

Attachment: stack of crash from VS debugger

Comment 32

[Bill Gianopoulos [:WG9s]]

Based on comment #12, #24 and the fact that my builds do not crash, I would say this definitely smells like bug 361343 to me.  I am going to do a new build with the code form that bug added to my .mozconfig and report back.

Comment 33

[Bill Gianopoulos [:WG9s]]

Bug 361343 is definitely the culprit.  Adding those 3 lines to my .mozconfig results in a build which crashes when trying to print that jpeg image.

Comment 34

[Bill Gianopoulos [:WG9s]]

This crash is related to cairo and jpeg.  Both of these are in C and not c++. So, I tired removing just this line:

export CFLAGS="-GL"

and leaving these 2:

export CXXFLAGS="-GL"
export LD_FLAGS="-LTCG"
 
That seems to be sufficient to avoid the crash.

Comment 35

[(not currently active) Ted Mielczarek]

That's just masking the problem though.  There's an actual bug here, exposed by the greater optimization.

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, from the stack:

>	xul.dll!_moz_cairo_surface_finish(_cairo_surface * surface=0x00000000)  Line 517 + 0x8 bytes	C
 	xul.dll!_cairo_win32_surface_release_source_image(void * abstract_surface=0x03626ae8, _cairo_image_surface * image=0x03782b70, void * image_extra=0x0012ed80)  Line 592 + 0x18 bytes	C

_cairo_win32_surface_release_source_image calls cairo_surface_destroy() on image_extra if it's not NULL.

cairo_surface_destroy decrements the reference count, and if it's 0, it calls cairo_surface_finish on the passed-in surface.

We're ending up in cairo_surface_finish with a surface of NULL, when it wasn't NULL in _cairo_win32_surface_release_source_image.  cairo_surface_destroy also does a NULL check on surface right at the start and returns if it's NULL, and that's not being triggered.  (This is all assuming that we can trust the argument values in that function call, though.)

So something's not right here, especially since cairo_surface_finish /also/ does a null check of surface right at the start.  Line 517 is:

    status = surface->backend->finish (surface);

which indeed would blow up if surface or backend was NULL, but there were many references to both surface and backend in that function previously and they're not blowing up.

Comment 37

[Bill Gianopoulos [:WG9s]]

(In reply to comment #36)
> So something's not right here, especially since cairo_surface_finish /also/
> does a null check of surface right at the start.  Line 517 is:
> 
>     status = surface->backend->finish (surface);
> 
> which indeed would blow up if surface or backend was NULL, but there were many
> references to both surface and backend in that function previously and they're
> not blowing up.
> 

The code in that area does not look that complicated.  This smells like a compiler bug to me.

Comment 38

[Robert Sayre]

(In reply to comment #37)
>
> The code in that area does not look that complicated.  This smells like a
> compiler bug to me.

I might agree, if not for attachment 296877 [details]. If we don't solve this in a few days, we'll give cairo a -GL- option to turn it off for that module.

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This shouldn't be too hard to figure out from stepping through the assembly, assuming you can catch that cairo_win32_surface_release_source_image reliably before things blow up.  I wish we had some better tools to trace data flow at the instruction/register level while debugging.

Comment 40

[Robert Sayre]

This can be avoided by compiling without WPO in jpeg/ cairo/cairo and cairo/libpixman. I think this is a bug with the GL option and inline asm. Could be related to this issue:

http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=99487

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That claims that the issue was fixed in SP1 (VS2005 SP1, I assume).  Are the tinderboxes running SP1?

Comment 42

[Bill Gianopoulos [:WG9s]]

(In reply to comment #40)
> This can be avoided by compiling without WPO in jpeg/ cairo/cairo and
> cairo/libpixman. I think this is a bug with the GL option and inline asm. Could
> be related to this issue:
> 
> http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=99487
> 
Well, if we are to do that, shouldn't we also make sure breakpad is not compiled with the GL option either as I believe it also has inline asm code.

Comment 43

[(not currently active) Ted Mielczarek]

(In reply to comment #42)
> Well, if we are to do that, shouldn't we also make sure breakpad is not
> compiled with the GL option either as I believe it also has inline asm code.

Uh, no.  Only the Mac and Linux client code has inline assembly.

Comment 44

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We have inline assembly in other chunks of the code though; e.g. xpconnect.  The bug described there doesn't exist in SP1, which we're building with.  There may still be problems with -GL when inline assembly is present though, but I don't know how much faith I have in an optimization flag that breaks down in that way.

Comment 45

[Robert Sayre]

(In reply to comment #44)
> We have inline assembly in other chunks of the code though; e.g. xpconnect. 
> The bug described there doesn't exist in SP1, which we're building with.

Did anyone verify that? IIRC, the message in there was a little confusing.

Comment 46

[Robert Sayre]

OK, looking at the release date of SP1 and the comments there, seems obvious that it's in.

Comment 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, I verified by taking the code that reproduces the problem from that bug report and testing that it works with SP1.

Comment 49

[Samuel Sidler (old account; do not CC)]

(In reply to comment #38)
> If we don't solve this in a few days, we'll give cairo a -GL- option
> to turn it off for that module.

I'm going to request that, given it's been a few days now, we go with this option.

Is there any good reason why the changes in bug 361343 weren't backed out while work was being done to figure out this bug? Given the amount of people that are seeing this (the _moz_cairo_surface_finish signature alone is topcrash #6), it seems logical to have backed that change out until a fix was determined.

Comment 50

[(not currently active) Ted Mielczarek]

Yes please, I just crashed there again.  This makes trunk nightlies hard to dogfood.  Let's either back out the original while you fix it, or disable -GL in those modules.  sayrer: can we land *something* today?

Comment 51

[Robert Sayre]

(In reply to comment #49)
> 
> Is there any good reason why the changes in bug 361343 weren't backed out while
> work was being done to figure out this bug? 

Yes, we want to find bugs in the product related to bug 361343. There could have been more.

Comment 52

[Bill Gianopoulos [:WG9s]]

Do we have the capability to override this on a per module basis without causing build bustage for people building with other compilers?

That is kind of why I suggested only doing the -GL for the c++ code, and not the c code in comment #34.

Comment 53

[Robert Sayre]

Attachment: turn off WPO in areas discussed at firefox meeting

Comment 54

[Robert Sayre]

Attachment: turn off WPO in areas discussed at firefox meeting v2

Comment 55

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011900 Minefield/3.0b3pre ID:2008011900

remaining ?
for printpreview - the printpreview popup stays open with message "preparing", this only seems to happen for a first printpreview, the next time it works.

this doesn't happen all the time, sometimes it closes after a while

Comment 56

[Bill Gianopoulos [:WG9s]]

(In reply to comment #55)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011900
> Minefield/3.0b3pre ID:2008011900
> 
> remaining ?
> for printpreview - the printpreview popup stays open with message "preparing",
> this only seems to happen for a first printpreview, the next time it works.
> 
> this doesn't happen all the time, sometimes it closes after a while
> 
This happens every time for me.

I have verified that this does not happen with my own builds which are not compiled with -GL.

Comment 57

[Bill Gianopoulos [:WG9s]]

Interestingly, I cannot reproduce this issue with my own build compiled with the WPO stuff enabled.

Comment 58

[Bill Gianopoulos [:WG9s]]

(In reply to comment #57)
> Interestingly, I cannot reproduce this issue with my own build compiled with
> the WPO stuff enabled.
> 

Major differences in my build would be compiling using vc8 express and using cygwin and not mozillabuild.

Comment 59

[Robert Sayre]

Finding problems printing attachment 296882 [details] in valgrind. Purify hasn't been much help--going to try some alternative tools on Windows.

Comment 60

[Mats Palmgren (inactive)]

(In reply to comment #55)
> remaining ?
> for printpreview - the printpreview popup stays open with message "preparing"

See bug 413200 comment 11

Comment 61

[Bill Gianopoulos [:WG9s]]

(In reply to comment #60)
> (In reply to comment #55)
> > remaining ?
> > for printpreview - the printpreview popup stays open with message "preparing"
> 
> See bug 413200 comment 11
> 
That would explain why I could not duplicate the issues in my builds.  I include the patch for bug 412598 which fixes the problems introduced by bug  	352791.

Comment 62

[Robert Sayre]

The patches in bug 413169 and bug 413170 might fix this crash, but I still have my tinfoil hat on. Marking this fixed, someone can take bug 413019 if they really want WPO turned back on for gfx. There are FIXME comments in all the right places.

Comment 63

[Martijn Wargers (dead)]

Yes, thanks, I can print again with current trunk build.
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre

Comment 64

[Michael Moy]

I have a PGO Win64 build running with -GL back in for PNG, GFX (including libpixman) where print works. JPEG is still a problem in jdsamples.

Comment 65

[Michael Moy]

(In reply to comment #42)
> (In reply to comment #40)
> > This can be avoided by compiling without WPO in jpeg/ cairo/cairo and
> > cairo/libpixman. I think this is a bug with the GL option and inline asm. Could
> > be related to this issue:
> > 
> > http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=99487
> > 
> Well, if we are to do that, shouldn't we also make sure breakpad is not
> compiled with the GL option either as I believe it also has inline asm code.

The Microsoft support note indicates that the problem is with mmx in the inline assembly around the emms instruction. The workaround is to use the equivalent intrinsic. JPEG only uses mmx in the jidctfst code which isn't used by Mozilla