445707 - BadValue in XShmPutImage with windowless Flash

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Karl Tomlinson (:karlt)]

STR:

* Install flash player 10 beta 2 (d525)

* View
  http://26dayswithoutaniphone.com/index.php/2008/07/14/more-pictures-from-the-weekend/
  on a local X display.

* Scroll down so that the large video is visible

* Cover the bottom right-corner of the video with the top-left corner of
  another window.

* Drag the covering window down

Results:

BadValue (integer parameter out of range for operation)

It seems that XShmPutImage produces the BadValue error when the subimage does
not fit within the destination drawable.  (XPutImage handles such cases
without error and transfers the appropriate portion of the image as expected.)

#0  _XError (dpy=0x8062628, rep=0xbf91e520) at ../../src/XlibInt.c:2874
#1  0xb73246c4 in _XReply (dpy=0x8062628, rep=0xbf91e520, extra=0, discard=1)
    at ../../src/XlibInt.c:1833
#2  0xb731e5da in XSync (dpy=0x8062628, discard=0) at ../../src/Sync.c:48
#3  0xb731e755 in _XSyncFunction (dpy=0x8062628) at ../../src/Synchro.c:37
#4  0xb74c162b in XShmPutImage (dpy=0x8062628, d=48240934, gc=0x94c9ec8,
    image=0x928c540, src_x=403, src_y=262, dst_x=-1, dst_y=0, src_width=198,
    src_height=1, send_event=0) at ../../src/XShm.c:358
#5  0xb755664a in gdk_x11_draw_image (drawable=0x8c2cd80, gc=0x9104408,
    image=0x9104360, xsrc=403, ysrc=262, xdest=-1, ydest=0, width=198,
    height=1) at /build/buildd/gtk+2.0-2.12.0/gdk/x11/gdkdrawable-x11.c:795
#6  0xb752e2a2 in IA__gdk_draw_image (drawable=0x8c2cd80, gc=0x9104408,
    image=0x9104360, xsrc=403, ysrc=262, xdest=-1, ydest=0, width=198,
    height=1) at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
#7  0xb75398bc in gdk_pixmap_draw_image (drawable=0x86126a8, gc=0x9104408,
    image=0x9104360, xsrc=403, ysrc=262, xdest=-1, ydest=0, width=198,
    height=1) at /build/buildd/gtk+2.0-2.12.0/gdk/gdkpixmap.c:407
#8  0xb752e2a2 in IA__gdk_draw_image (drawable=0x86126a8, gc=0x9104408,
    image=0x9104360, xsrc=403, ysrc=262, xdest=-1, ydest=0, width=198,
    height=1) at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
#9  0xac788bb0 in ?? () from /home/karl/flash/d525/libflashplayer.so
#10 0xac77cd8a in ?? () from /home/karl/flash/d525/libflashplayer.so
#11 0xac773d80 in ?? () from /home/karl/flash/d525/libflashplayer.so
#12 0xac7788b4 in Private_HandleEvent ()
   from /home/karl/flash/d525/libflashplayer.so
#13 0xad1e56e0 in ns4xPluginInstance::HandleEvent (this=0x94d4128,
    event=0xbf91e86c, handled=0xbf91e85c)
    at /home/karl/moz/dev/modules/plugin/base/src/ns4xPluginInstance.cpp:1310
#14 0xad71f95b in nsPluginInstanceOwner::Renderer::NativeDraw (
    this=0xbf91ed1c, screen=0x8060ab8, drawable=48240934, visual=0x8067780,
    colormap=32, offsetX=-403, offsetY=-262, clipRects=0xbf91e968,
    numClipRects=1) at /home/karl/moz/dev/layout/generic/nsObjectFrame.cpp:4214

Plugins do not get told the dimensions of the drawable, but they do get told
the dirty rect on the drawable, so it would seem reasonable for plugins to
assume that drawing to the dirty rect would be safe.  However the dirty rect
provided to the plugin in the expose event is not on the drawable:

(gdb) f 14
#14 0xad71f95b in nsPluginInstanceOwner::Renderer::NativeDraw (
    this=0xbf91ed1c, screen=0x8060ab8, drawable=48240934, visual=0x8067780,
    colormap=32, offsetX=-403, offsetY=-262, clipRects=0xbf91e968,
    numClipRects=1) at /home/karl/moz/dev/layout/generic/nsObjectFrame.cpp:4214
4214      mInstance->HandleEvent(&pluginEvent, &eventHandled);
(gdb) p exposeEvent
$1 = (._182 &) @0xbf91e86c: {type = 13, serial = 0, send_event = 0,
  display = 0x8062628, drawable = 48240934, x = -1, y = 0, width = 198,
  height = 1, count = 0, major_code = 0, minor_code = 0}

(gdb) f 8
#8  0xb752e2a2 in IA__gdk_draw_image (drawable=0x86126a8, gc=0x9104408,
    image=0x9104360, xsrc=403, ysrc=262, xdest=-1, ydest=0, width=198,
    height=1) at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
703     /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c: No such file or directory.
        in /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c
(gdb) p gdk_x11_drawable_get_xid(drawable)
$3 = 48240934

(gdb) f 16
#16 0xb2218543 in _draw_with_xlib_direct (cr=0x91fd578,
    default_display=0x8062628, callback=0xb2237dc0 <NativeRendering>,
    closure=0xbf91ec58, bounds_width=600, bounds_height=600, capabilities=27)
    at /home/karl/moz/dev/gfx/thebes/src/cairo-xlib-utils.c:312
312         callback (closure, screen, d, visual, offset_x, offset_y, rectangles,
(gdb) p ((cairo_xlib_surface_t*)target)->width
$5 = 197
(gdb) p ((cairo_xlib_surface_t*)target)->height
$6 = 1

The dirty rect given to the plugin comes from rounding the app-unit dirty rect
out to pixels.  The reason why the app unit dirty rect is not pixel aligned is
because nsDisplayClip::Paint for the nsSubDocumentFrame intersects the
unsnapped clip rect with the dirty rect.

I don't yet know why the dirty rect is larger than the surface when there is
no rotation involved.
 
Perhaps a similar situation may have been possible before the fix for bug
430450, but the rounding out of the dirty rect in that fix makes this
situation much more likely.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: clip the dirty rect

The simple fix is to intersect the dirty rect with the clip rect.
_draw_with_xlib_direct provides a clip rect when the drawable is smaller than
the plugin rect, and the dirty rect is always smaller than the plugin rect.
_draw_onto_temp_xlib_surface always has a surface the size of the plugin rect.

Plugins should only be drawing within the clip rect but providing a dirty rect
greater than the clip rect doesn't make much sense.  I think a plugin should
be able to assume that it can draw to the dirty rect.  (The plugin interface
doesn't provide for clip regions.)

Comment 2

[Karl Tomlinson (:karlt)]

I think I was jumping to conclusions regarding nsDisplayClip::Paint().

Bug 445932 is the reason why the app unit dirty rect is not aligned to pixels and extends outside the surface.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: clip the dirty rect and fix up a comment

I think there is enough uncertainty about pixel rounding that it's worth taking this to be safe, even though the test case here doesn't show any problem with bug 445932 fixed.

Also, it would be nice to fix this on 1.9.0 because we want bug 430450 fixed on 1.9.0.  Taking this patch on 1.9.0 instead of the fix for bug 445932 would be safer just because this patch only affects windowless plugins.

Comment 4

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

Can't you use nsRect::IntersectRect here?

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: clip the nsIntRect dirty rect

(In reply to comment #4)

Yes, that does make it easier to get the logic right ;)

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/af91b0f5f3e3

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 330528 [details] [diff] [review]
clip the nsIntRect dirty rect

Requesting approval to land with the fix for bug 430450.  Adobe are working around the issue with later plugins but there are still some older Beta plugins around.

Comment 8

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

Comment on attachment 330528 [details] [diff] [review]
clip the nsIntRect dirty rect

Approved for 1.9.0.2. Please land in CVS when the tree is green. a=ss

Comment 9

[Karl Tomlinson (:karlt)]

Landed in CVS:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1218776280&maxdate=1218776342&who=karlt%2B%25karlt.net

Comment 10

[Karl Tomlinson (:karlt)]

The change here was essentially reverted in bug 422221 and reinstated in bug 526797.  That means this is not actually fixed on 1.9.1, but AFAIK it is not causing any significant problems beyond painting an extra row/column of pixels in some unusual situations, because Adobe has a workaround for the XShm issues in their more recent Flash Player.

Added a mochitest, which relies on the checks from bug 537301:
http://hg.mozilla.org/mozilla-central/rev/b0e993f4f07e
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7f85397aa446

Comment 11

[hallchris]

So here some of the of the bug that i have seen Mozilla Firefox if u clearify this bug its relly helpful for the user so check out .https://goo.gl/Qn9WnW

Comment 12

[hallchris]

So we know that getting error is the mistake has been get when we enter the wrong details so here https://goo.gl/awk7c1 so tht every one can get some idea according to this site research we have done in the apst about Mozilla https://goo.gl/YZsaBf.So when you want to learn some thing new from our site then here u can learn about https://goo.gl/WRTJdA