469831 - need gtk2 drawing code for test plugin

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

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

Comment 1

[Josh Aas]

mventnor - any chance you could do this?

Comment 2

[Michael Ventnor]

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?

Comment 3

[Josh Aas]

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

Comment 4

[Josh Aas]

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

Comment 5

[Michael Ventnor]

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

Comment 6

[Michael Ventnor]

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

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Mac screenshot

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.

Comment 8

[Michael Ventnor]

Attachment: Patch

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Michael Ventnor]

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

Comment 11

[(not currently active) Ted Mielczarek]

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

Comment 12

[Michael Ventnor]

Attachment: Patch 2

Adds support for RGBA fill colours using the same interface.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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);

Comment 14

[Michael Ventnor]

Attachment: Patch 3

Moved that code.

Comment 15

[(not currently active) Ted Mielczarek]

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.)

Comment 16

[Michael Ventnor]

(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?

Comment 17

[(not currently active) Ted Mielczarek]

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

Comment 18

[(not currently active) Ted Mielczarek]

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

Comment 19

[(not currently active) Ted Mielczarek]

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.

Comment 20

[(not currently active) Ted Mielczarek]

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);

Comment 21

[Michael Ventnor]

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

Comment 22

[Michael Ventnor]

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

Comment 23

[(not currently active) Ted Mielczarek]

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

Comment 24

[(not currently active) Ted Mielczarek]

Attachment: for consideration (not for checkin)

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.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 26

[Karl Tomlinson (:karlt)]

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);

Comment 27

[Zack Weinberg (:zwol)]

Attachment: patch using reinterpret_cast<XID>

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

Comment 28

[Karl Tomlinson (:karlt)]

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.

Comment 29

[(not currently active) Ted Mielczarek]

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

Comment 30

[(not currently active) Ted Mielczarek]

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

Comment 31

[Marco Bonardo [:mak]]

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!

Comment 32

[Karl Tomlinson (:karlt)]

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

Comment 33

[(not currently active) Ted Mielczarek]

(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.

Comment 34

[Michael Ventnor]

Attachment: Patch with bustage fix

This seems to fix it on my end (but then again so did my ALSA message suppression patch...)

ted, could you check this in?

Comment 35

[Karl Tomlinson (:karlt)]

>    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 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment #31 suggests that mochitests can reproduce this...

I need to get this landed to make progress on compositor stuff.

Comment 37

[(not currently active) Ted Mielczarek]

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

Comment 38

[Michael Ventnor]

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

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fixed patch

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.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The try-servers report no crash with this patch.

Comment 43

[Karl Tomlinson (:karlt)]

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.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 45

[Karl Tomlinson (:karlt)]

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.)

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

As I suggested...

Comment 49

[Karl Tomlinson (:karlt)]

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.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded.

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

Comment 51

[(not currently active) Ted Mielczarek]

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.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 53

[(not currently active) Ted Mielczarek]

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.

Comment 54

[Karl Tomlinson (:karlt)]

(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.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.