Last Comment Bug 722975 - --enable-system-cairo build is broken after Bug 715658 fixed
: --enable-system-cairo build is broken after Bug 715658 fixed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 5 votes (vote)
: mozilla24
Assigned To: ojab
:
:
Mentors:
: 880216 (view as bug list)
Depends on:
Blocks: gtk3 715658 795737 798926
  Show dependency treegraph
 
Reported: 2012-01-31 21:25 PST by ojab
Modified: 2014-07-03 07:00 PDT (History)
31 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fail in configure when --enable-system-cairo is used (882 bytes, patch)
2012-02-02 20:35 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
ted: review+
Details | Diff | Splinter Review
Ad 2 #ifdefs to gfx/thebes/gfxPlatform.cpp (1.04 KB, patch)
2012-02-07 05:28 PST, Andrew Benton
no flags Details | Diff | Splinter Review
patch the new functions into cairo-1.10.2 (14.08 KB, patch)
2012-02-07 12:57 PST, Andrew Benton
no flags Details | Diff | Splinter Review
patch the new functions into cairo-1.12.0 (16.98 KB, patch)
2012-03-27 14:51 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
patch the new functions into cairo-1.12.0 (16.42 KB, patch)
2012-03-31 11:53 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
Make system cairo work again (1.94 KB, patch)
2012-05-16 08:42 PDT, Uli Schlachter
no flags Details | Diff | Splinter Review
minimal testcase - html (554 bytes, text/plain)
2012-06-06 05:54 PDT, Rafał Mużyło
no flags Details
minimal testcase - css (130 bytes, text/plain)
2012-06-06 05:55 PDT, Rafał Mużyło
no flags Details
minimal testcase - html (corrected) (550 bytes, text/plain)
2012-06-06 06:10 PDT, Rafał Mużyło
no flags Details
a screenshot of the site (118.51 KB, image/png)
2012-06-06 09:57 PDT, Rafał Mużyło
no flags Details
Updated patch (2.09 KB, patch)
2012-08-01 10:46 PDT, ojab
no flags Details | Diff | Splinter Review
Updated patch (2.19 KB, patch)
2012-10-09 10:51 PDT, ojab
bas: review+
Details | Diff | Splinter Review
Patch with ifdefs (1.45 KB, patch)
2013-05-23 12:28 PDT, Connor Behan
no flags Details | Diff | Splinter Review
Patch with ifdefs (1.46 KB, patch)
2013-05-23 21:27 PDT, Connor Behan
bas: review+
Details | Diff | Splinter Review

Description ojab 2012-01-31 21:25:29 PST
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp: In member function ‘virtual mozilla::RefPtr<mozilla::gfx::SourceSurface> gfxPlatform::GetSourceSurfaceForSurface(mozilla::gfx::DrawTarget*, gfxASurface*)’:
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp:516:53: error: ‘cairo_null_surface_create’ was not declared in this scope
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp:521:95: error: ‘cairo_surface_attach_snapshot’ was not declared in this scope
make[7]: *** [gfxPlatform.o] Error 1
Comment 1 Andrew Benton 2012-02-02 03:57:58 PST
Confirmed, I'm also seeing this bug. If I revert the patch attached to bug 715658 then the build succeeds, another option is to remove --enable-system-cairo from my mozconfig; otherwise the build fails with the error above.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-02-02 07:39:54 PST
When I was reviewing bug 715658 I took the fact that it would break system cairo into account. System-cairo is not really a supported configuration and I decided that a quick, and reliably correct solution was better than trying to avoid exposing the new api to cairo. That being said, our reliance on this API should be temporary and when we don't need it anymore I'd like to get system cairo working again. I expect, I'd also take a patch that removed our dependency on this API. In the meantime we should probably make configure fail when --enable-system-cairo is specified.
Comment 3 Bas Schouten (:bas.schouten) 2012-02-02 20:35:17 PST
Created attachment 594066 [details] [diff] [review]
Fail in configure when --enable-system-cairo is used

This should be the patch suggested by jrmuizel.
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-02-02 20:42:22 PST
Comment on attachment 594066 [details] [diff] [review]
Fail in configure when --enable-system-cairo is used

Looks fine to me but I don't read autoconf very well. If you're not sure about it's correctness please get a review from a better reviewer.
Comment 5 Octoploid 2012-02-02 23:29:40 PST
(In reply to Bas Schouten (:bas) from comment #3)
> Created attachment 594066 [details] [diff] [review]
> Fail in configure when --enable-system-cairo is used
> 
> This should be the patch suggested by jrmuizel.

Isn't this a bit drastic, when a few simple ifdefs in gfx/thebes/gfxPlatform.cpp
would be sufficient to fix this problem?
Comment 6 Andrew Benton 2012-02-07 05:28:06 PST
Created attachment 595000 [details] [diff] [review]
Ad 2 #ifdefs to gfx/thebes/gfxPlatform.cpp

(In reply to Octoploid from comment #5)
> (In reply to Bas Schouten (:bas) from comment #3)
> Isn't this a bit drastic, when a few simple ifdefs in
> gfx/thebes/gfxPlatform.cpp
> would be sufficient to fix this problem?

Simple? I can do simple! This patch fixes compiling with --enable-system-cairo for me
Comment 7 Bas Schouten (:bas.schouten) 2012-02-07 06:18:29 PST
(In reply to Andrew Benton from comment #6)
> Created attachment 595000 [details] [diff] [review]
> Ad 2 #ifdefs to gfx/thebes/gfxPlatform.cpp
> 
> (In reply to Octoploid from comment #5)
> > (In reply to Bas Schouten (:bas) from comment #3)
> > Isn't this a bit drastic, when a few simple ifdefs in
> > gfx/thebes/gfxPlatform.cpp
> > would be sufficient to fix this problem?
> 
> Simple? I can do simple! This patch fixes compiling with
> --enable-system-cairo for me

It means this bug exists again on linux (once we switch Azure content rendering on there), so it doesn't actually help.
Comment 8 Andrew Benton 2012-02-07 07:38:58 PST
(In reply to Bas Schouten (:bas) from comment #7)
> It means this bug exists again on linux (once we switch Azure content
> rendering on there), so it doesn't actually help.

It means I can compile firefox again which is a step in the right direction. The alternative is to stick with Firefox-10 for the foreseeable future.
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-02-07 07:42:11 PST
(In reply to Andrew Benton from comment #8)
> (In reply to Bas Schouten (:bas) from comment #7)
> > It means this bug exists again on linux (once we switch Azure content
> > rendering on there), so it doesn't actually help.
> 
> It means I can compile firefox again which is a step in the right direction.
> The alternative is to stick with Firefox-10 for the foreseeable future.

Why can't you compile firefox without system cairo?
Comment 10 Andrew Benton 2012-02-07 08:14:49 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> Why can't you compile firefox without system cairo?

Pure stubborn bloodymindedness ;)
I prefer to link to system libraries wherever possible
Comment 11 Jeff Muizelaar [:jrmuizel] 2012-02-07 08:20:54 PST
(In reply to Andrew Benton from comment #10)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > Why can't you compile firefox without system cairo?
> 
> Pure stubborn bloodymindedness ;)
> I prefer to link to system libraries wherever possible

If that's the reason, you can always apply this patch yourself :)
Comment 12 Octoploid 2012-02-07 08:42:53 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Andrew Benton from comment #8)
> > (In reply to Bas Schouten (:bas) from comment #7)
> > > It means this bug exists again on linux (once we switch Azure content
> > > rendering on there), so it doesn't actually help.
> > 
> > It means I can compile firefox again which is a step in the right direction.
> > The alternative is to stick with Firefox-10 for the foreseeable future.
> 
> Why can't you compile firefox without system cairo?

There was a time when Firefox, compiled without system cairo, looked 
**** (color fringes), because the in-tree cairo only included the awful 
LCD_FILTER_LEGACY algorithm. 
I've been using --enable-system-cairo since that time.
But today I've build Firefox without --enable-system-cairo and it actually looks OK now.
So --enable-system-cairo isn't necessary anymore.
Comment 13 Andrew Benton 2012-02-07 12:57:44 PST
Created attachment 595138 [details] [diff] [review]
patch the new functions into cairo-1.10.2

Well if the mountain will not come to Mohammed, perhaps we can patch cairo to have the new functions? This patch adds the new functions into cairo-1.10.2. With the new functions in the system cairo I can compile firefox without patching the firefox code
Comment 14 Ted Mielczarek [:ted.mielczarek] 2012-02-14 05:08:17 PST
Comment on attachment 594066 [details] [diff] [review]
Fail in configure when --enable-system-cairo is used

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

::: configure.in
@@ +8066,5 @@
>  
>  MOZ_TREE_CAIRO=1
>  MOZ_ARG_ENABLE_BOOL(system-cairo,
>  [  --enable-system-cairo   Use system cairo (located with pkgconfig)],
> +AC_MSG_ERROR([ --enable-system-cairo is no longer supported.]),

Please file a bug on making system cairo work again, and list that bug number in a comment here (and possibly in the error message).
Comment 15 Ionut Biru 2012-03-20 13:25:28 PDT
doesn't mozilla like to push changes into upstream? 
this way the fix is for real rather than trying to apply workarounds in packages from distros or switching to internal cairo.
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-03-20 13:45:39 PDT
(In reply to Ionut Biru from comment #15)
> doesn't mozilla like to push changes into upstream? 
> this way the fix is for real rather than trying to apply workarounds in
> packages from distros or switching to internal cairo.

I've been hesitant to push this upstream because I'm not really sure it's appropriate.
Comment 17 Ionut Biru 2012-03-20 13:49:43 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> (In reply to Ionut Biru from comment #15)
> > doesn't mozilla like to push changes into upstream? 
> > this way the fix is for real rather than trying to apply workarounds in
> > packages from distros or switching to internal cairo.
> 
> I've been hesitant to push this upstream because I'm not really sure it's
> appropriate.

only one way to find out. let upstream review the changes and try harder to push it there if the review is not favorable. from that, mozilla has more benefits along with it,other communities as well.
Comment 18 Andrew Benton 2012-03-27 14:51:00 PDT
Created attachment 609898 [details] [diff] [review]
patch the new functions into cairo-1.12.0

Updated the system cairo patch for cairo-1.12.0
Comment 19 Andrew Benton 2012-03-28 04:35:44 PDT
(In reply to Andrew Benton from comment #18)
> Created attachment 609898 [details] [diff] [review]
> patch the new functions into cairo-1.12.0
> 
> Updated the system cairo patch for cairo-1.12.0

Ugh, that patch doesn't work, don't use it. Too much has changed between cairo-1.10.2 and 1.12.0. I don't understand enough to make it work.
Comment 20 Andrew Benton 2012-03-31 11:53:31 PDT
Created attachment 611180 [details] [diff] [review]
patch the new functions into cairo-1.12.0

Improved patch for cairo-1.12.0. This patch works in that I can compile Firefox with --enable-system-cairo. cairo-1.12.0 is pretty buggy though https://bugs.freedesktop.org/show_bug.cgi?id=47266
https://bugs.gentoo.org/show_bug.cgi?id=409593
Comment 21 Rafał Mużyło 2012-04-03 14:05:34 PDT
@comment 20: for the moment, it seems most of the problem (in regard of those bugs) lies in xorg-server, not cairo.
Comment 22 Landry Breuil (:gaston) 2012-04-18 15:01:38 PDT
Oh noes, not the "let's ditch --enable-system-cairo" argument again... i'll use https://bugzilla.mozilla.org/attachment.cgi?id=595000 in the meantime if it works, but it'd be good to have this properly fixed, as lots of distributors are using that switch. Fx 12.0b5 is broken for me as-is. Jeff, you're saying "our reliance on this API should be temporary", what's the timeframe/consequences ?
Comment 23 Mike Hommey [:glandium] 2012-04-18 15:09:27 PDT
(In reply to Andrew Benton from comment #20)
> Created attachment 611180 [details] [diff] [review]
> patch the new functions into cairo-1.12.0
> 
> Improved patch for cairo-1.12.0. This patch works in that I can compile
> Firefox with --enable-system-cairo. cairo-1.12.0 is pretty buggy though
> https://bugs.freedesktop.org/show_bug.cgi?id=47266
> https://bugs.gentoo.org/show_bug.cgi?id=409593

Note that the bugs lie on the X server side, not on the cairo side. Which means that whenever we upgrade cairo in the mozilla codebase, we'll hit those X server bugs :-/
Comment 24 Jeff Muizelaar [:jrmuizel] 2012-04-18 15:19:30 PDT
(In reply to Landry Breuil from comment #22)
> Jeff,
> you're saying "our reliance on this API should be temporary", what's the
> timeframe/consequences ?

Basically, we need to port the some of the image code to use Azure directly. Once, that happens we won't need to use private APIs anymore and can re-enable system-cairo support. I'm not sure if we'll be able to do this before we switch everything over to Azure or not. The timeframe depends largely on this.
Comment 25 Michael Shigorin 2012-05-08 04:36:13 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> System-cairo is not really a supported configuration
Wow, and I thought it was only Chromium guys being crazy enough to fork anything they touch with Firefox team being more prudent in terms of effort and output... hope it's still so.
Comment 26 Martin Stránský 2012-05-16 06:13:34 PDT
The breakage of system-cairo support breaks out GTK3 Firefox port too, because it requires to use system cairo (all GTK3 widgets are rendered by system cairo). Can we help somehow with the changes mentioned in comment 24? "to port the some of the image code to use Azure directly".
Comment 27 Martin Stránský 2012-05-16 06:36:44 PDT
Anyway, there's another regression with --enable-system-cairo:

gmake[6]: Entering directory `/home/komat/tmp646-trunk/src/objdir/layout/media'
/home/komat/tmp646-trunk/src/config/rules.mk:745: *** SHARED_LIBRARY_LIBS must contain .a files only.  Stop.

because with --enable-system-cairo the SHARED_LIBRARY_LIBS contains cairo dynamic libraries.
Comment 28 Mike Hommey [:glandium] 2012-05-16 06:50:50 PDT
(In reply to Martin Stránský from comment #27)
> Anyway, there's another regression with --enable-system-cairo:
> 
> gmake[6]: Entering directory
> `/home/komat/tmp646-trunk/src/objdir/layout/media'
> /home/komat/tmp646-trunk/src/config/rules.mk:745: *** SHARED_LIBRARY_LIBS
> must contain .a files only.  Stop.
> 
> because with --enable-system-cairo the SHARED_LIBRARY_LIBS contains cairo
> dynamic libraries.

It's the same as bug 751521. You may want to kill the two birds with the same stone.
Comment 29 Uli Schlachter 2012-05-16 08:42:17 PDT
Created attachment 624394 [details] [diff] [review]
Make system cairo work again

This patch makes mozilla build here with system cairo again. It replaces usage of cairo_surface_attach_snapshot() and _create_nil_surface() with public cairo API (cairo_surface_set_mime_data()). Mime data gets detached at the same time that snapshots get detached (see _cairo_surface_begin_modification()).
Comment 30 Nagy Gabor 2012-06-03 20:35:00 PDT
Comment on attachment 624394 [details] [diff] [review]
Make system cairo work again

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

As I see, your patch breaks the correct rendering of the following page (press End): http://rohejorszaga.blog.hu/2012/06/03/erre_egyszeruen_nem_talalok_szavakat_601?fullcommentlist=1#comments
Comment 31 Rafał Mużyło 2012-06-04 20:41:47 PDT
@comment 30:
Well, it seems even holding PageDown for awhile triggers the problem (it probably reverts to the state of bug 715658).
So, anyone up to asking *cairo* upstream how would they solve the original problem within existing *public* API ?
Or is the attachment 624394 [details] [diff] [review] the correct solution and it's firefox, that needs a bit stricter resource management ?
Comment 32 Mike Hommey [:glandium] 2012-06-04 22:01:12 PDT
(In reply to Rafał Mużyło from comment #31)
> @comment 30:
> Well, it seems even holding PageDown for awhile triggers the problem (it
> probably reverts to the state of bug 715658).
> So, anyone up to asking *cairo* upstream how would they solve the original
> problem within existing *public* API ?
> Or is the attachment 624394 [details] [diff] [review] the correct solution
> and it's firefox, that needs a bit stricter resource management ?

I don't know what specific version of firefox and cairo you use, but it works fine for me with cairo 1.12 and firefox 14. However, cairo 1.12 is known to expose X.org driver problems.
Comment 33 Rafał Mużyło 2012-06-05 02:10:54 PDT
@comment 30: most of it is a miss.
I *am* using cairo 1.12, but I'm also using the drivers with the fix for that problem (or at least that part of it that was easily exposed in firefox - basic acceleration of solid pictures via scratch 1x1 pixmaps). I'm using (as of yesterday) firefox 13 though, so perhaps it's fixed in 14 already.

Well, that is unless you're talking about a different set of cairo/X driver problems.
Comment 34 Mike Hommey [:glandium] 2012-06-05 02:16:45 PDT
(In reply to Rafał Mużyło from comment #33)
> @comment 30: most of it is a miss.
> I *am* using cairo 1.12, but I'm also using the drivers with the fix for
> that problem (or at least that part of it that was easily exposed in firefox
> - basic acceleration of solid pictures via scratch 1x1 pixmaps). I'm using
> (as of yesterday) firefox 13 though, so perhaps it's fixed in 14 already.

Works fine here in firefox 13 as well.

> Well, that is unless you're talking about a different set of cairo/X driver
> problems.

AFAIK, there are several different problems, not all of which are fixed ATM.
Comment 35 Rafał Mużyło 2012-06-05 13:19:28 PDT
(In reply to Mike Hommey [:glandium] from comment #34)
> (In reply to Rafał Mużyło from comment #33)
> > Well, that is unless you're talking about a different set of cairo/X driver
> > problems.
> 
> AFAIK, there are several different problems, not all of which are fixed ATM.

Can you share the bug numbers in upstream bugzilla or ... ?
Comment 36 Andrew Benton 2012-06-05 17:01:47 PDT
(In reply to Rafał Mużyło from comment #35)
> (In reply to Mike Hommey [:glandium] from comment #34)
> > (In reply to Rafał Mużyło from comment #33)
> > > Well, that is unless you're talking about a different set of cairo/X driver
> > > problems.
> > 
> > AFAIK, there are several different problems, not all of which are fixed ATM.
> 
> Can you share the bug numbers in upstream bugzilla or ... ?

I did in comment #20
Comment 37 Rafał Mużyło 2012-06-06 04:56:05 PDT
(In reply to Andrew Benton from comment #36)
> (In reply to Rafał Mużyło from comment #35)
> > Can you share the bug numbers in upstream bugzilla or ... ?
> 
> I did in comment #20
Your info is out of date (and there were a few of my comments in both of those bugs).
The Gentoo bug was exactly the same problem as the freedesktop one and the later one has long been fix in all three major driver branches (intel, ati and nouveau).
Comment 38 Rafał Mużyło 2012-06-06 05:54:33 PDT
Created attachment 630531 [details]
minimal testcase - html

...and on a related note: I do notice an odd case of display corruption, though I'm not quite sure whether it's related to cairo 1.12.

I've managed to reduce it into a nearly trivial test case.
To trigger the corruption put html and css files in one folder, and hover the mouse over those two links. As you switch a few times between them, backrground of the links becomes corrupted.
Comment 39 Rafał Mużyło 2012-06-06 05:55:59 PDT
Created attachment 630532 [details]
minimal testcase - css
Comment 40 Mike Hommey [:glandium] 2012-06-06 05:59:11 PDT
(In reply to Rafał Mużyło from comment #38)
> Created attachment 630531 [details]
> minimal testcase - html
> 
> ...and on a related note: I do notice an odd case of display corruption,
> though I'm not quite sure whether it's related to cairo 1.12.
> 
> I've managed to reduce it into a nearly trivial test case.
> To trigger the corruption put html and css files in one folder, and hover
> the mouse over those two links. As you switch a few times between them,
> backrground of the links becomes corrupted.

Still no problem here. Can we just stop chasing cairo or Xorg bugs here anyways?
Comment 41 Rafał Mużyło 2012-06-06 06:10:33 PDT
Created attachment 630538 [details]
minimal testcase - html (corrected)

Minor correction of the testcase (now it validates as xhtml).
Comment 42 Rafał Mużyło 2012-06-06 09:14:52 PDT
This bug became a bit of a mess, so lets summarize:
While using patch from comment 29:
- that link problem happens only in cairo built with '--enable-xlib-xcb', so my mistake there (that option is disabled by default)
- problem in bug 715658 happens regardless of cairo version, so patch might be incomplete
- problem from comment 30 happens only with cairo 1.12 - I'll see what upstream bugzilla has to say
Comment 43 Mike Hommey [:glandium] 2012-06-06 09:20:57 PDT
(In reply to Rafał Mużyło from comment #42)
> - problem in bug 715658 happens regardless of cairo version, so patch might
> be incomplete

With https://bugzilla.mozilla.org/attachment.cgi?id=624394, it doesn't. The patch is no incomplete.
Comment 44 Rafał Mużyło 2012-06-06 09:57:58 PDT
Created attachment 630607 [details]
a screenshot of the site

With both cairo 1.10.2 and 1.12.2, http://scoop.today.msnbc.msn.com/_news/2012/01/05/9977921-rooney-mara-girl-with-the-style-we-want-to-steal looks like on the attached screenshot while using the mentioned patch.

I don't think that black rectangle is the correct display.
Comment 45 Mike Hommey [:glandium] 2012-06-06 10:03:56 PDT
(In reply to Rafał Mużyło from comment #44)
> Created attachment 630607 [details]
> a screenshot of the site
> 
> With both cairo 1.10.2 and 1.12.2,
> http://scoop.today.msnbc.msn.com/_news/2012/01/05/9977921-rooney-mara-girl-
> with-the-style-we-want-to-steal looks like on the attached screenshot while
> using the mentioned patch.
> 
> I don't think that black rectangle is the correct display.

And again, this has nothing to do with this bug, and works fine here. Can we stop here?
Comment 46 ojab 2012-08-01 10:46:52 PDT
Created attachment 648007 [details] [diff] [review]
Updated patch

Guys, can it be committed?
Comment 47 ojab 2012-08-12 23:15:53 PDT
ping?
Comment 48 Connor Behan 2012-08-28 23:45:56 PDT
Every EXA driver is different but in my case, text looks much better with --enable-system-cairo. If a program is designed to use a version of cairo that has 91 patches applied, this doesn't seem right to me.
Comment 49 ojab 2012-09-16 06:05:46 PDT
Four month since there is a working patch with review request, even longer build with system cairo is broken.
Can anybody review/commit the patch?
Comment 50 Connor Behan 2012-09-17 16:07:01 PDT
This patch is being pushed by people who think it is inefficient for mozilla to try to fork cairo. However, mozilla has now decided to go with the even more inefficient route of writing a vector graphics library from scratch to replace cairo. Maybe that's why they aren't committing this patch.
Comment 51 thierry.vignaud 2012-10-01 02:44:36 PDT
Also status "UNCONFIRMED" is obviously false...
Comment 52 ojab 2012-10-09 10:51:22 PDT
Created attachment 669619 [details] [diff] [review]
Updated patch

And, after nine month since the bug was filled, the patch has bitrotted once again.
:/
Comment 53 Bas Schouten (:bas.schouten) 2012-10-09 11:06:29 PDT
Comment on attachment 669619 [details] [diff] [review]
Updated patch

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

If this works lets land it. We need to keep a very close eye if it doesn't break anything though.
Comment 54 ojab 2012-10-09 11:12:49 PDT
Hooray!
Comment 55 Ryan VanderMeulen [:RyanVM] 2012-10-09 18:17:49 PDT
I don't see any Try results here, so I've gone ahead and started a run myself.
https://tbpl.mozilla.org/?tree=Try&rev=034a386f0a57

Uli, sorry for the extra delay, but given Bas' comments, I want to make sure it at least passes Mozilla's internal testsuite before pushing it. I will push it as soon as the Try comes back OK.
Comment 56 Ryan VanderMeulen [:RyanVM] 2012-10-10 03:54:11 PDT
Green on Try.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2231b8e778a3

Uli, thanks again for the patch! Just so you know where things are in the process, this patch has landed on mozilla-inbound, which is an intermediate integration branch. It is merged to mozilla-central (the official repo) once or twice daily. Once it lands on m-c, this bug will be resolved as FIXED. At this point, it will land for Firefox 19. If you want, you can request approval on the patch for aurora/beta to get it in an earlier release, but I can't promise you that the request will be accepted. Thanks again!
Comment 57 Ed Morley [:emorley] 2012-10-10 04:54:10 PDT
(In reply to Ryan VanderMeulen from comment #56)
> Green on Try.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2231b8e778a3

Unfortunately this just tested Linux & this caused failures on other platforms, so I've had to back it out.

Windows doesn't compile with this patch - linking errors, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15982013&tree=Mozilla-Inbound

Tests on OS X crash, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15982181&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=15982231&tree=Mozilla-Inbound

Full results:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=dee0f227e656

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa702386c7c
Comment 58 Uli Schlachter 2012-10-10 11:12:56 PDT
No clue what the heck is wrong on windows. cairo-rename.h handles cairo_surface_set_mime_data() the same way it handles other cairo functions and there is nothing windows-specific in there. So why does this only happen on windows?

For the OS X crashes:
No good idea here either. The stack trace looks bogous. _cairo_mime_data_destroy is supposed to call the function pointer given to cairo_surface_set_mime_data(), so this is supposed to end up in SourceSnapshotDetached. However, for some weird reason, _cairo_user_data_array_set_data() is called instead. Weird memory corruption?
Comment 59 ojab 2012-10-10 11:18:01 PDT
Looks like windows build is broken because of https://hg.mozilla.org/integration/mozilla-inbound/rev/27c51f14c3ed
Comment 60 Ed Morley [:emorley] 2012-10-11 07:07:10 PDT
https://hg.mozilla.org/mozilla-central/rev/05006509aa53
Comment 61 Ryan VanderMeulen [:RyanVM] 2012-10-11 07:09:47 PDT
This was backed out, no?
Comment 62 Ryan VanderMeulen [:RyanVM] 2012-10-11 07:11:50 PDT
Ah crap, that changeset had the wrong bug number in it. The changeset in comment #60 belongs to bug 779209.
Comment 63 Bas Schouten (:bas.schouten) 2012-10-23 11:45:55 PDT
Comment on attachment 624394 [details] [diff] [review]
Make system cairo work again

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

Afaiui this doesn't work yet.
Comment 64 Connor Behan 2012-11-22 15:34:46 PST
Which build is broken on Windows? The --enable-system-cairo one that didn't work before or --disable-system-cairo? If it's the latter, the problem can still be fixed temporarily using ifdefs right? This would beat having to wait hours for a -Wl,--no-keep-memory build to finish.
Comment 65 Rafał Mużyło 2013-04-21 17:45:55 PDT
I's like bring a little corner case to your attention, that comes from using patch from attachment 624394 [details] [diff] [review] - bug 862422. It might be hard to tell whether the problem lies on cairo or libxul side. Also, the conditions to trigger the case are very specific, but it's not being established what they exactly are.
Comment 66 Connor Behan 2013-05-23 12:28:50 PDT
Created attachment 753419 [details] [diff] [review]
Patch with ifdefs
Comment 67 Jory A. Pratt 2013-05-23 19:09:04 PDT
(In reply to Connor Behan from comment #66)
> Created attachment 753419 [details] [diff] [review]
> Patch with ifdef

The ifdefs should not be against linux but rather against MOZ_TREE_CAIRO.
Comment 68 Timothy Nikkel (:tnikkel) 2013-05-23 19:10:41 PDT
Comment on attachment 753419 [details] [diff] [review]
Patch with ifdefs

Reviews without a person attached to them are likely to be ignored, best to choose someone, even if you aren't sure they can redirect the review request.
Comment 69 Connor Behan 2013-05-23 21:27:37 PDT
Created attachment 753610 [details] [diff] [review]
Patch with ifdefs

Yeah, that makes more sense.
Comment 70 Takanori MATSUURA 2013-05-26 21:08:03 PDT
(In reply to Connor Behan from comment #69)
BTW, switching by tree or not is a good idea?
If this is depended by the version of cairo, we should use it.
Comment 72 Phil Ringnalda (:philor) 2013-05-31 21:42:28 PDT
https://hg.mozilla.org/mozilla-central/rev/52b02042b27f
Comment 73 Jan Beich 2014-04-05 02:49:53 PDT
*** Bug 880216 has been marked as a duplicate of this bug. ***

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