We need some gtk2 drawing code for the test plugin. The Mac OS X implementation already has drawing code. Relevant files: modules/plugin/test/testplugin/nptest_platform.h modules/plugin/test/testplugin/nptest_gtk2.cpp modules/plugin/test/testplugin/nptest_macosx.mm

mventnor - any chance you could do this?

I can't find any of the three files mentioned above in a just-checked-out m-c. Do I need to do any additional steps?

I had to back it out due to leaks, I'll re-land soon.

Michael - the test plugin is landed again and it stuck.

Could you please attach a screenshot of the expected output of the Mac plugin? Also, how am I able to test this plugin?

roc said he'd upload a screenshot, and I figured out how to load it.

IMHO if you want to test plugin rendering we should not be drawing something unpredictable like UA string text. Instead, the test plugins should draw some solid color regions or something that we can easily compare to a reference.

roc: see bug 473577. ventron: if you can sync up with the patch on that bug, that would be great!

Will the patch currently in bug 473577 land soon or will it need reviews/more revisions?

Josh seemed ok with the concept, so I'd write to interoperate with the data structures there.

Adds support for RGBA fill colours using the same interface.

+ float r,g,b,a; + GetColorsFromRGBA(instanceData->scriptableObject->drawColor, &r, &g, &b, &a); + cairo_set_source_rgba(cairoWindow, r, g, b, a); It'd be slightly cleaner to make the helper function set the cairo color directly, so SetCairoColorFromRGBA(cairoWindow, instanceData->scriptableObject->drawColor);

Moved that code.

If cairo gets color values as floats too, we could move that helper function from my patch into the _utils.cpp. (I didn't know if it would be usable cross-platform.)

(In reply to comment #15) > If cairo gets color values as floats too, we could move that helper function > from my patch into the _utils.cpp. (I didn't know if it would be usable > cross-platform.) Well, we did just add a cairo call into that function for ourself. Plus IIRC Windows uses ints for each channel, doesn't it?

Dunno. Not a real big deal, it's only a few lines of code.

My stuff landed, no significant changes. You will have to tweak modules/plugin/test/reftest/reftest.list though.

Actually, my linux tree is broken right now, and I have to head out shortly, so you'll have to find someone else to land it. Sorry! Just update the reftest.list and set checkin-needed.

This fails to compile on x86-64 for me: /home/luser/build/mozilla-central/modules/plugin/test/testplugin/nptest_gtk2.cpp: In function ‘void pluginDraw(InstanceData*)’: /home/luser/build/mozilla-central/modules/plugin/test/testplugin/nptest_gtk2.cpp:122: error: cast from ‘void*’ to ‘GdkNativeWindow’ loses precision after some googling, this seems to make it compile, but I have to #include <stdint.h>, and I'm not sure I can unilaterally do that: GdkNativeWindow nativeWinId = (GdkNativeWindow)reinterpret_cast<uintptr_t>(window.window);

Hmm, I wonder if a GPOINTER_TO_INT() would fix that, but I don't have a 64-bit machine to find out.

Or, since its unsigned, GPOINTER_TO_UINT. Of course then theres the problem of whether an XID is the same size as a guint...

On the plus side, with that change the reftests pass on my x86-64 box.

Here's that patch with my hack to get it to compile on x86-64, and the reftest manifest updated to run those tests on GTK. Clearly we need a better solution here. Ventron: if you've got any ideas, I'm open to trying them out.

I'm pretty sure Karl can sort this out :-)

I would suggest casting to XID as that is what is really represented in the void* window.window. The weird thing is that this works (without any explicit intermediate cast) because XID is a long unsigned int on the client side: /usr/include/X11/X.h: /* * _XSERVER64 must ONLY be defined when compiling X server sources on * systems where unsigned long is not 32 bits, must NOT be used in * client or library code. */ #ifndef _XSERVER64 # ifndef _XTYPEDEF_XID # define _XTYPEDEF_XID typedef unsigned long XID; # endif ... #else # include <X11/Xmd.h> # ifndef _XTYPEDEF_XID # define _XTYPEDEF_XID typedef CARD32 XID; # endif ... #endif GdkNativeWindow is only a guint32 (unsigned int), but this happens to be fine because XIDs are never too large to be represented by 32 bits. So either of the following works on systems where a long is large enough to hold a pointer: GdkNativeWindow nativeWinId = XID(window.window); GdkNativeWindow nativeWinId = reinterpret_cast<XID>(window.window);

So, like this maybe? Works on my amd64-linux box.

Yes, thanks. I spoke with Michael on this, and we both like that the XID cast clearly indicates the nature of the data. The only LLP64 system that I know of is Win64 (bug 226094 comment 2 is interesting). If a GTK/X11 plugin becomes important on Win64 or some other LLP64 system comes up, then we can switch to GPOINTER_TO_UINT at that stage.

WFM as well. Thanks for the fixup, let's get this landed.

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/92a47ed3d54e

backed out due to mochitests leak failures: http://hg.mozilla.org/mozilla-central/rev/7581b9caf300 http://hg.mozilla.org/mozilla-central/rev/4119594ac66d (merge) http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234215799.1234221087.7877.gz TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!

Gecko: Fatal IO error 9 (Bad file descriptor) on X server :2.0.

(In reply to comment #32) > Gecko: Fatal IO error 9 (Bad file descriptor) on X server :2.0. Right. This wasn't a leak failure, there was an X error after Mochitest finished, which caused a shutdown crash, so the leak log wasn't written.

This seems to fix it on my end (but then again so did my ALSA message suppression patch...) ted, could you check this in?

> XGraphicsExposeEvent *expose = &nsEvent->xgraphicsexpose; > + if (!expose->display || !expose->drawable) > + return 0; > + Neither display nor drawable should be 0. Do you have some steps to reproduce this, as it looks like a bug?

Comment #31 suggests that mochitests can reproduce this... I need to get this landed to make progress on compositor stuff.

I meant to ping this bug to see where we were. Michael: can you consult with Karl and sort this out?

I haven't been able to reproduce the problem, and I don't have any time right now to work on it. Sorry.

I can reproduce a crash after the test plugin has rendered. Here's the stack trace #0 0xb7493b84 in XRenderFindDisplay () from /usr/lib/libXrender.so.1 #1 0xb749070f in XRenderFreeGlyphs () from /usr/lib/libXrender.so.1 #2 0xb76b8cee in cairo_xlib_surface_get_display () from /usr/lib/libcairo.so.2 #3 0xb76988f7 in cairo_scaled_font_get_font_matrix () from /usr/lib/libcairo.so.2 #4 0xb768e080 in cairo_create () from /usr/lib/libcairo.so.2 #5 0xb768e201 in cairo_create () from /usr/lib/libcairo.so.2 #6 0xb7698f8b in cairo_scaled_font_get_font_matrix () from /usr/lib/libcairo.so.2 #7 0xb76992f4 in cairo_scaled_font_destroy () from /usr/lib/libcairo.so.2 #8 0xb768f3c2 in cairo_font_face_status () from /usr/lib/libcairo.so.2 #9 0xb768eff0 in cairo_debug_reset_static_data () from /usr/lib/libcairo.so.2 #10 0xb7e7f3b3 in MOZ_gdk_display_close (display=0xb6c63040) at /home/roc/shared/mozilla-checkin/toolkit/xre/nsAppRunner.cpp:2415 #11 0xb7e867dd in XRE_main (argc=5, argv=0xbfae4cf4, aAppData=0xb6c0e380) at /home/roc/shared/mozilla-checkin/toolkit/xre/nsAppRunner.cpp:3329 #12 0x08048f31 in main (argc=5, argv=0xbfae4cf4) at /home/roc/shared/mozilla-checkin/browser/app/nsBrowserApp.cpp:156 These symbols seem bogus but tragically I can't find any debug symbol packages for OpenSUSE 10.2 out on the Web anymore. I'd upgrade except maybe then I wouldn't be able to reproduce the crash...

I spoke too soon. Here's a real stack trace: #0 XRenderFindDisplay (dpy=0xb6c3a000) at Xrender.c:125 #1 0xb755270f in XRenderFreeGlyphs (dpy=0xb6c3a000, glyphset=41945130, gids=0xbfaf1d40, nglyphs=1) at Glyph.c:121 #2 0xb777acee in _cairo_xlib_surface_scaled_glyph_fini (scaled_glyph=0xb6c3a000, scaled_font=0xaddd58c0) at cairo-xlib-surface.c:2300 #3 0xb775a8f7 in _cairo_scaled_glyph_destroy (abstract_glyph=0xadd25520) at cairo-scaled-font.c:59 #4 0xb7750080 in _cairo_cache_remove (cache=0xad0bcd00, entry=0xadd25520) at cairo-cache.c:350 #5 0xb7750201 in _cairo_cache_destroy (cache=0xad0bcd00) at cairo-cache.c:94 #6 0xb775af8b in _cairo_scaled_font_fini (scaled_font=0xaddd58c0) at cairo-scaled-font.c:412 #7 0xb775b2f4 in _cairo_scaled_font_map_destroy () at cairo-scaled-font.c:245 #8 0xb77513c2 in _cairo_font_reset_static_data () at cairo-font.c:470 #9 0xb7750ff0 in cairo_debug_reset_static_data () at cairo-debug.c:66 #10 0xb7f413b3 in MOZ_gdk_display_close (display=0xb6c62040) at /home/roc/shared/mozilla-checkin/toolkit/xre/nsAppRunner.cpp:2415 #11 0xb7f487dd in XRE_main (argc=5, argv=0xbfaf2504, aAppData=0xb6c0e380) at /home/roc/shared/mozilla-checkin/toolkit/xre/nsAppRunner.cpp:3329 #12 0x08048f31 in main (argc=5, argv=0xbfaf2504) at /home/roc/shared/mozilla-checkin/browser/app/nsBrowserApp.cpp:156

This seems to fix the crash for me. It looks like we're crashing cleaning up a font that has a pointer to a destroyed Display. I thought Display objects were immortal but it appears not. If we close the display *after* cleaning up cairo here, we don't crash in my build. I think this probably is the right fix. Ultimately we're falling victim to the fact that there is no way to clean out fonts from cairo's font cache when things they depend on go away. We have this problem with library unloads in other contexts IIRC.

The try-servers report no crash with this patch.

I'm trying to imagine how attachment 360636 [details] [diff] [review] could cause chrome, browser, and mochitests to have a "Gecko: Fatal IO error 9 (Bad file descriptor) on X server :2.0.". The patch only modifies code that is run only when the test plugin is rendered, and AFAICS the text plugin is only used in the chrome tests, not browser or mochitest.

Actually it's used in browser tests and mochitest, see http://mxr.mozilla.org/mozilla-central/search?string=application%2Fx-test It's used in reftests too, but only in solid-color mode so the Pango code isn't hit.

Comment on attachment 368859 [details] [diff] [review] fixed patch (In reply to comment #44) > Actually it's used in browser tests and mochitest, see > http://mxr.mozilla.org/mozilla-central/search?string=application%2Fx-test > It's used in reftests too, but only in solid-color mode so the Pango code > isn't hit. Ah, thanks. That does seem to implicate the text code. And I could imagine both the crash in comment 40 and "Fatal IO error 9 (Bad file descriptor) on X server :2.0" resulting from access to a closed display. (In reply to comment #41) > It looks like we're crashing cleaning up a font that has a pointer to a > destroyed Display. I thought Display objects were immortal but it appears not. Display objects are not immortal, but cairo installs close-display hooks to clean up when the Displays are closed. It is possible (perhaps likely) that there are some versions of cairo that don't get the clean-up right. > If we close the display *after* cleaning up cairo here, we don't crash in my > build. There was a reason for the order of shutdown. gdk depends on pango, which depends on cairo. Shutting down pango and cairo before removing our last reference to gdk feels a bit risky, but perhaps gdk has finished with the other libraries at this point. I assume that trace-malloc leak tests run on similar systems, and this code is mostly here for leak detection, so disabling for certain versions of cairo is not helpful. I guess we give the change in shutdown order a try. The expectation of a dependency of gdk on pango and cairo was the reason why pango and cairo were not being shutdown when gdk_display_close is avoided due to gtk_check_version(2,10,0) failing, but that was a theoretical reason. If we are going to shut down pango and cairo before gdk then we might as well shut them down regardless of the gdk version (before the gtk_check_version(2,10,0)). I'm wondering what is special about the plugin's use of fonts, as gtk uses gdk_draw_layout in file save dialogs at least. I can't see anything in the test plugin that might cause a problem. Do you see the same shutdown crash after a File -> Save Page As? If you see a crash there, then the plugin is not special. If you don't see a crash, then that would suggest that the plugin is doing something peculiar. + // Close the display *after* we're cleared out cairo's static data, to + // avoid problems with dangling display pointers. This shouldn't be + // necessary, but is. "but is with some versions of cairo. (See bug 469831.)" (This doesn't seem to be a problem with recent versions of cairo.)

Yes, I do actually see the same shutdown crash after bringing up the Save dialog.

I appear to have system cairo 1.2.4 (!) and that's what Centos5 also appears to have. How about we leave the current code as-is and I'll add an extra code path for cairo < 1.4.0 which shuts down cairo last to work around this presumed cairo bug?

As I suggested...

Comment on attachment 369042 [details] [diff] [review] fix v2 (In reply to comment #47) > How about we leave the current code as-is and I'll add an extra code path > for cairo < 1.4.0 which shuts down cairo last to work around this presumed > cairo bug? That makes sense. This looks a little weird when CLEANUP_MEMORY is not set (as in release builds): > #endif // CLEANUP_MEMORY > >+ PRBool buggyCairoShutdown = cairo_version() < CAIRO_VERSION_ENCODE(1, 4, 0); >+ >+ if (!buggyCairoShutdown) { >+ if (!theme_is_qt) >+ gdk_display_close(display); >+ } > > #if CLEANUP_MEMORY > #endif // CLEANUP_MEMORY >+ >+ if (buggyCairoShutdown) { >+ if (!theme_is_qt) >+ gdk_display_close(display); >+ } > } > } I guess the compiler will notice and the cairo_version() call will be the only unnecessary code. Carefully moving preprocessor directives around could be an option but that gets a little messy when they cross block boundaries anyway. I'll let you decide.

Relanded. http://hg.mozilla.org/mozilla-central/rev/4629633764e5

I added this reftest in bug 484780: fails-if(!haveTestPlugin||(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows")) == plugin-alpha-zindex.html div-alpha-zindex.html and your checkin didn't fix that. I'm surprised that's not causing orange, unless the alpha rendering code in the gtk2 plugin doesn't work right.

Your test assumes that drawing an rgba(r,g,b,a) color directly is the same as drawing rgb(r,g,b) to a temporary surface and then compositing that onto a target using alpha=a, which isn't true in general due to rounding. In this case you've chosen the numbers so that it *should* work out (since 0.6 is exactly 153/255), but I'm not completely sure. I'm not really sure what's going on. Probably needs a followup bug.

I did in fact choose those numbers to give me an exact decimal and byte value opacity equivalent. I don't know how our rendering actually works, just that it passed with the OS X drawing code that I wrote. Feel free to fix the test or otherwise work out what's happening there.

(In reply to comment #51) > unless the alpha rendering code in the gtk2 plugin doesn't work right. This might be related to bug 486250.

My checkin for bug 484923 caused that test to start passing, because I started setting the transparent NPPVpluginTransparentBool when a translucent color was specified, so I updated reftest.list to reflect that.