682625 - Crash, null pointer deref [@ mozilla::plugins::PluginInstanceParent::BackgroundDescriptor ]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is one of the topcrashers on Firefox 7.0b1 / Linux, however all the reports last week seem to originate from a single user:

https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A7.0b1&platform=linux&query_search=signature&query_type=contains&reason_type=contains&date=&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Aplugins%3A%3APluginInstanceParent%3A%3ABackgroundDescriptor

Here's one crash:

https://crash-stats.mozilla.com/report/index/1ef2a10d-5fee-4106-8e28-2beea2110822

We're dereferencing null here:

http://hg.mozilla.org/releases/mozilla-beta/annotate/00f56713eab6/dom/plugins/ipc/PluginInstanceParent.cpp#l831

We have:

NS_ABORT_IF_FALSE(mBackground, "Need a background here");
gfxXlibSurface* xsurf = static_cast<gfxXlibSurface*>(mBackground.get());
return SurfaceDescriptorX11(xsurf->XDrawable(), xsurf->XRenderFormat()->id, xsurf->GetSize()); // <-- null deref here

Looks like mBackground was null and this wasn't caught by the NS_ABORT_IF_FALSE because it's a release build.

Last touched by cjones in bug 626602, http://hg.mozilla.org/releases/mozilla-beta/rev/dae302e05a8b

Comment 1

[Karl Tomlinson (:karlt)]

Could be the same as bug 646150, where xsurf->XRenderFormat() is null.

Build id 20110816154714 is consistent with
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/releases/7.0b1/linux-i686/en-US/firefox-7.0b1.tar.bz2
so it doesn't seem to be an Ubuntu build.

Comment 2

[Karl Tomlinson (:karlt)]

The zero offset from the null pointer confirms that it is xsurf->XRenderFormat() that is null.

Comment 3

[Robert Kaiser]

Found no crashes with that signature within a week, but looking beyond, there's a couple: https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozilla%3A%3Aplugins%3A%3APluginInstanceParent%3A%3ABackgroundDescriptor

The ones on 7.0 smell very much like a single user, given the install times matches to the second, the one on 7.0a2 and the one on 8.0a1 seems to be two other users that have seen it.

Comment 4

[Robert Kaiser]

20 on 8.0* over 4 weeks and few on other versions, one even on 11.0a1, so still valid, but not a topcrash.

Comment 5

[cloos]

I confirm that this bug is still there, as described, in ff901 and sm261.

It is triggered by plugins (I rarely use them, but do need to use a couple of sites which require flash); as soon as the plugin loads the browser invariably crashes.

I use sm and ff on amd64 Linux boxen.

Comment 6

[cloos]

I forgot to write:

Given when this started occurring, it seems likely to be related to the switch to sandbox plugins.

Is there and quick and easy way to back out the plugin sandboxing so as to confirm whether the bug still exists w/o that?

Comment 7

[cloos]

And from a core dump, showing that in this cases xsurf->msize has valid height and width values.  I tried this once w/ ff8 compiled -O0 and, IIRC, the hight and width were 0....

(gdb) up
#3  mozilla::plugins::PluginInstanceParent::BackgroundDescriptor (this=<optimized out>)
    at /usr/src/debug/www-client/seamonkey-2.6.1/comm-release/mozilla/dom/plugins/ipc/PluginInstanceParent.cpp:874
874	                                xsurf->GetSize());
(gdb) p xsurf
$1 = (gfxXlibSurface *) 0x7f9bdc391c40
(gdb) p *xsurf
$2 = {<gfxASurface> = {_vptr.gfxASurface = 0x7f9c0f2d0c50, mSurface = 0x7f9bdcba6cc0, 
    mOpaqueRect = {mRawPtr = 0x0}, mFloatingRefs = 0, mBytesRecorded = 304800, 
    mSurfaceValid = 1 '\001', mAllowUseAsSource = 1 '\001'}, mPixmapTaken = 1, 
  mDisplay = 0x7f9c01649000, mDrawable = 41954458, 
  mSize = {<mozilla::gfx::BaseSize<int, nsIntSize>> = {width = 400, 
      height = 254}, <No data fields>}, mGLXPixmap = 0}

Comment 8

[Bill Gianopoulos [:WG9s]]

Bug 714695 seems related, but the crash seems different.

Comment 9

[Karl Tomlinson (:karlt)]

cloos: would you be able to print "xsurf->XRenderFormat()" as well as 
"*((Visual*(*)())cairo_xlib_surface_get_visual)(xsurf->mSurface)",
and attach output from xdpyinfo -ext RENDER please?

Are you building with --enable-system-cairo or the default disable-system-cairo?

Comment 11

[cloos]

I re-compiled ff with -O0 to get a better debug.

I turns out that some swf content displays OK.  Eg the GIS site:

  http://gis.co.chautauqua.ny.us:8080/parcels/default.htm

while others (such as a finance site I must access) triggers the segv.

I use Gentoo’s ebuild to build; so --enable-system-cairo.  My cairo is git master.

I’ll kept the gdb session around for a while for further analysis, if needed.

Program received signal SIGSEGV, Segmentation fault.
mozilla::plugins::PluginInstanceParent::BackgroundDescriptor (this=0x7fffcd8ed6c0) at /usr/src/debug/www-client/firefox-9.0/mozilla-release/dom/plugins/ipc/PluginInstanceParent.cpp:874
874                                     xsurf->GetSize());

(gdb) print "xsurf->XRenderFormat()"
$1 = "xsurf->XRenderFormat()"

(gdb) print xsurf->XRenderFormat()
$2 = (XRenderPictFormat *) 0x0

(gdb) p *((Visual*(*)())cairo_xlib_surface_get_visual)(xsurf->mSurface)
$3 = {ext_data = 0x0, visualid = 33, c_class = 4, red_mask = 16711680, green_mask = 65280, blue_mask = 255, bits_per_rgb = 8, map_entries = 256}

(gdb) p *xsurf
$4 = {<gfxASurface> = {_vptr.gfxASurface = 0x7ffff6753330, mSurface = 0x7fffcd254320, mOpaqueRect = {mRawPtr = 0x0}, mFloatingRefs = 0, mBytesRecorded = 304800, mSurfaceValid = 1 '\001',
    mAllowUseAsSource = 1 '\001'}, mPixmapTaken = 1, mDisplay = 0x7ffff6da8000, mDrawable = 41944439, mSize = {<mozilla::gfx::BaseSize<int, nsIntSize>> = {width = 400, height = 254}, <No data fields>},
  mGLXPixmap = 0}

so msize has valid width and height, but XRenderFormat() returns NULL.

Comment 12

[Karl Tomlinson (:karlt)]

cloos, i have a Gentoo build but haven't seen the crash.
Can you attach output from "xdpyinfo -ext RENDER", please?

Comment 13

[Ren Ren-Juan]

name of display:    :1.0
version number:    11.0
vendor string:    AT&T Laboratories Cambridge
vendor release number:    3332
maximum request size:  4194300 bytes
motion buffer size:  256
bitmap unit, bit order, padding:    32, LSBFirst, 32
image byte order:    LSBFirst
number of supported pixmap formats:    2
supported pixmap formats:
    depth 1, bits_per_pixel 1, scanline_pad 32
    depth 24, bits_per_pixel 32, scanline_pad 32
keycode range:    minimum 8, maximum 255
focus:  window 0x2800002, revert to Parent
number of extensions:    7
    BIG-REQUESTS
    MIT-SHM
    MIT-SUNDRY-NONSTANDARD
    SHAPE
    SYNC
    XC-MISC
    XTEST
default screen number:    0
number of screens:    1

screen #0:
  dimensions:    1920x1200 pixels (650x406 millimeters)
  resolution:    75x75 dots per inch
  depths (1):    24
  root window id:    0x25
  depth of root window:    24 planes
  number of colormaps:    minimum 1, maximum 1
  default colormap:    0x21
  default number of colormap cells:    256
  preallocated pixels:    black 0, white 16777215
  options:    backing-store YES, save-unders YES
  largest cursor:    1920x1200
  current input event mask:    0xda3f3f
    KeyPressMask             KeyReleaseMask           ButtonPressMask          
    ButtonReleaseMask        EnterWindowMask          LeaveWindowMask          
    Button1MotionMask        Button2MotionMask        Button3MotionMask        
    Button4MotionMask        Button5MotionMask        ButtonMotionMask         
    StructureNotifyMask      SubstructureNotifyMask   SubstructureRedirectMask 
    PropertyChangeMask       ColormapChangeMask       
  number of visuals:    1
  default visual id:  0x22
  visual:
    visual id:    0x22
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff0000, 0xff00, 0xff
    significant bits in color specification:    8 bits

RENDER extension not supported by server

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Ren Ren-Juan from comment #13)

Thanks.

> vendor string:    AT&T Laboratories Cambridge

Is this the X server shipped with Debian Squeeze?
Or is this a VNC connection?

> RENDER extension not supported by server

Do you know why this is?

Comment 15

[Ren Ren-Juan]

Yes that's apparently it. I start FF in fvwm session which is started at the commmand line via fvwm being set as the VNC wm. I then access that either thru KDE or locally but only locally for some time. Running in just KDE I was unable to reproduce it.

Comment 16

[cloos]

Attachment: xdpyinfo -ext RENDER

Sorry, forgot that.  Here it is.

Given its size and my limited net bw, I had to compress and attach it.

Comment 17

[Karl Tomlinson (:karlt)]

Comment 5 to 7 are due to a system cairo built with --enable-xlib_xcb.  With this backend, cairo_xlib_surface_get_xrender_format returns a format only if the surface was created with a format.
The cairo_xcb_surface_t may well have a PictFormat, but this is not translated
to a XRenderPictFormat, a struct that is not typically used with xcb.

http://cgit.freedesktop.org/cairo/tree/src/cairo-xlib-xcb-surface.c?id=3ebe0ca876c10425b88033683d7f85dcddcc09be#n583

Migrating from Xlib/libXrender to xcb is something we'll want to do at some stage, and using this cairo backend may be a sensible first step.  The fix for bug 714695 will also fix this situation.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: share code for layers::SurfaceDescriptorX11 with plugins

We can remove some duplicated code and fix this in one place instead of two.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: use simpler SurfaceDescriptorX11(gfxXlibSurface) constructor

Simpler code and necessary to have a single fix in SurfaceDescriptorX11.

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: move XVisualIDToInfo to X11Util

This method is a bit tidier than XMatchVisualInfo and doesn't require a memory allocation, so I'd like to use it in SurfaceDescriptorX11.

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: pass VisualID when PictFormat is not available

This is the actual fix for this bug.  The visual describes the Drawable format when there is no render format readily available.  I don't know for sure that X resources of different types will always have different XIDs, but the current implementation is that all resources get their XIDs from one pool.

There is some reordering of struct member variables here, which should pack better on 32-bit systems.  (On 64-bit systems, Xlib uses longs for XIDs even though they are 32-bit integers.)

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: pass VisualID when PictFormat is not available

This version is more robust against situations that shouldn't happen.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 586903 [details] [diff] [review]
share code for layers::SurfaceDescriptorX11 with plugins

>diff --git a/dom/plugins/ipc/PPluginInstance.ipdl b/dom/plugins/ipc/PPluginInstance.ipdl
> 
>-struct SurfaceDescriptorX11 {
>-  int XID;
>-  int xrenderPictID;
>-  gfxIntSize size;
>-};
>-

This makes me a little sad, but it's my fault for not fixing bug
521898.

>diff --git a/dom/plugins/ipc/PluginMessageUtils.h b/dom/plugins/ipc/PluginMessageUtils.h

>+#include "gfxipc/ShadowLayerUtils.h"

Huh I had no idea we had "gfxipc".  I wouldn't have r+'d that,
but not your problem :).

>+using mozilla::layers::SurfaceDescriptorX11;
>+

Can you pull this into mozilla::plugins to avoid polluting the
global namespace?

r=me with that fixed.  Nice cleanup!

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 586906 [details] [diff] [review]
move XVisualIDToInfo to X11Util

>diff --git a/gfx/src/X11Util.h b/gfx/src/X11Util.h

>+bool
>+XVisualIDToInfo(Display* aDisplay, VisualID aVisualID,
>+                Visual** aVisual, unsigned int* aDepth);
>+

Can you add a short comment describing this function?  Also the
ownership semantics of |aVisual|.

r=me with that.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 586911 [details] [diff] [review]
pass VisualID when PictFormat is not available

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsX11.h b/gfx/layers/ipc/ShadowLayerUtilsX11.h

>-  SurfaceDescriptorX11(const int aXid, const int aXrenderPictID, const gfxIntSize& aSize);
>+  SurfaceDescriptorX11(const int aXid, const int aFormatID, const gfxIntSize& aSize);
> 

I don't recall why these use |int| and not |XID|, but if you're
happy with it I'm happy with it :).  Looks like I r+'d this in
bug 687373.

Looks good.

Comment 27

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d3f721ffc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/04dd5995b086
https://hg.mozilla.org/integration/mozilla-inbound/rev/98768291cee9
https://hg.mozilla.org/integration/mozilla-inbound/rev/6571d631dd40

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e2d3f721ffc9
https://hg.mozilla.org/mozilla-central/rev/04dd5995b086
https://hg.mozilla.org/mozilla-central/rev/98768291cee9
https://hg.mozilla.org/mozilla-central/rev/6571d631dd40