With https://bugs.webkit.org/show_bug.cgi?id=64321 there can be a custom Cursor object with an empty Image. Encoding them would lead to a crash since a ShareableBitmap of size 0 would be requested. This returns a NULL and causes a crash. #0 0x00007f141d5ee05a in WebCore::IntSize::width (this=0x8) at ../../Source/WebCore/platform/graphics/IntSize.h:67 #1 0x00007f141d6051ca in WebKit::ShareableBitmap::createCairoSurface (this=0x0) at ../../Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:69 #2 0x00007f141d604ec9 in WebKit::ShareableBitmap::createGraphicsContext (this=0x0) at ../../Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:43 #3 0x00007f141d622f51 in CoreIPC::encodeImage (encoder=0x23a8650, image=0x23c3c50) at ../../Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:294
Created attachment 101340 [details] Handle the case of a custom cursor with a null image
Nate, what are your thoughts here?
(In reply to comment #2) > Nate, what are your thoughts here? It looks fine to me. I think this can be fixed in WebCore to ensure we don't send custom cursors with null Image*s to the embedder, but the patch will be much uglier than this one due to the #if(LAZY_NATIVE_CURSOR) flag.
Comment on attachment 101340 [details] Handle the case of a custom cursor with a null image This is wrong. It will encode something that the decoder will fail to decode. We need to encode something that indicates there is no image following, perhaps a boolean.
(In reply to comment #4) > (From update of attachment 101340 [details]) > This is wrong. It will encode something that the decoder will fail to decode. We need to encode something that indicates there is no image following, perhaps a boolean. Darin, would prefer this just fixed in WebCore to ensure we don't send null images? I'd be happy to write that patch, but it wouldn't get fixed today.
(In reply to comment #5) > Darin, would you prefer this just fixed in WebCore to ensure we don't send null images? I'd be happy to write that patch, but it wouldn't get fixed today. I think the right question is what behavior we want when such a cursor is specified. Once we know what behavior we want, then we can figure out how to get that behavior. We could start by figuring out what behavior we have here in WebKit1. I’m thinking that a custom cursor with an empty image should probably result in an invisible cursor. Is that right? As far as implementation is concerned, it’s OK with me to fix the encoding machinery so it can successfully send an empty custom cursor across the process boundary. I object to the incorrect patch, not the very notion of what the patch is doing. The patch is wrong because if we encode one of these, the decoder will then try to decode fields that aren’t present and consume additional bytes and thus be unable to decode the rest of the stream. That’s a relatively simple mechanical thing to fix.
(In reply to comment #6) > (In reply to comment #5) > > Darin, would you prefer this just fixed in WebCore to ensure we don't send null images? I'd be happy to write that patch, but it wouldn't get fixed today. > > I think the right question is what behavior we want when such a cursor is specified. Once we know what behavior we want, then we can figure out how to get that behavior. We could start by figuring out what behavior we have here in WebKit1. I’m thinking that a custom cursor with an empty image should probably result in an invisible cursor. Is that right? In WebKit1, GTK handles this case. In case a NULL image is sent to gdk_window_set_cursor, it uses cursor of the parent window to display. > > As far as implementation is concerned, it’s OK with me to fix the encoding machinery so it can successfully send an empty custom cursor across the process boundary. I object to the incorrect patch, not the very notion of what the patch is doing. The patch is wrong because if we encode one of these, the decoder will then try to decode fields that aren’t present and consume additional bytes and thus be unable to decode the rest of the stream. That’s a relatively simple mechanical thing to fix. I will attach a patch shortly addressing this.
Created attachment 101474 [details] Indicate the presence of a null image while encoding
It looks like individual ports needs to additionally handle this. QT port still crashes with my patch.
*** Bug 65420 has been marked as a duplicate of this bug. ***
Qt port crash with this patch: #0 0xb2e4d8c4 in operator! (this=0x0) at ../../include/QtCore/../../src/corelib/tools/qshareddata.h:198 #1 QPixmap::isNull (this=0x0) at image/qpixmap.cpp:545 #2 0xb2e4d90a in QPixmap::toImage (this=0x0) at image/qpixmap.cpp:484 #3 0xb2d78784 in QCursor::QCursor (this=0x82f1870, pixmap=..., hotX=0, hotY=0) at kernel/qcursor.cpp:313 #4 0xb5d22e2e in WebCore::createCustomCursor (image=0x82f1a08, hotSpot=...) at ../../../Source/WebCore/platform/qt/CursorQt.cpp:79 #5 0xb5d238de in WebCore::Cursor::ensurePlatformCursor (this=0xbfc484bc) at ../../../Source/WebCore/platform/qt/CursorQt.cpp:199 #6 0xb5d1ccf3 in WebCore::Cursor::platformCursor (this=0xbfc484bc) at ../../../Source/WebCore/platform/Cursor.cpp:263 #7 0xb54ae51f in QtWebPageProxy::setCursor (this=0x8283ec0, cursor=...) at ../../../Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:239 #8 0xb546f945 in WebKit::WebPageProxy::setCursor (this=0xaf702068, cursor=...) at ../../../Source/WebKit2/UIProcess/WebPageProxy.cpp:2666 #9 0xb5505535 in CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&), WebCore::Cursor> (args=..., object=0xaf702068, function= (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:19 #10 0xb55010e8 in CoreIPC::handleMessage<Messages::WebPageProxy::SetCursor, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&)> (argumentDecoder=0xaf7034c0, object=0xaf702068, function= (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:277 #11 0xb54fee14 in WebKit::WebPageProxy::didReceiveWebPageProxyMessage (this=0xaf702068, messageID=...,
Like I mentioned in comment #9, QT port needs to handle this case at the UI process. In case a null image is passed to GTK, it displays the cursor of parent window. (In reply to comment #11) > Qt port crash with this patch: > #0 0xb2e4d8c4 in operator! (this=0x0) at ../../include/QtCore/../../src/corelib/tools/qshareddata.h:198 > #1 QPixmap::isNull (this=0x0) at image/qpixmap.cpp:545 > #2 0xb2e4d90a in QPixmap::toImage (this=0x0) at image/qpixmap.cpp:484 > #3 0xb2d78784 in QCursor::QCursor (this=0x82f1870, pixmap=..., hotX=0, hotY=0) at kernel/qcursor.cpp:313 > #4 0xb5d22e2e in WebCore::createCustomCursor (image=0x82f1a08, hotSpot=...) > at ../../../Source/WebCore/platform/qt/CursorQt.cpp:79 > #5 0xb5d238de in WebCore::Cursor::ensurePlatformCursor (this=0xbfc484bc) > at ../../../Source/WebCore/platform/qt/CursorQt.cpp:199 > #6 0xb5d1ccf3 in WebCore::Cursor::platformCursor (this=0xbfc484bc) > at ../../../Source/WebCore/platform/Cursor.cpp:263 > #7 0xb54ae51f in QtWebPageProxy::setCursor (this=0x8283ec0, cursor=...) > at ../../../Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:239 > #8 0xb546f945 in WebKit::WebPageProxy::setCursor (this=0xaf702068, cursor=...) > at ../../../Source/WebKit2/UIProcess/WebPageProxy.cpp:2666 > #9 0xb5505535 in CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&), WebCore::Cursor> (args=..., object=0xaf702068, function= > (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:19 > #10 0xb55010e8 in CoreIPC::handleMessage<Messages::WebPageProxy::SetCursor, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&)> (argumentDecoder=0xaf7034c0, object=0xaf702068, function= > (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:277 > #11 0xb54fee14 in WebKit::WebPageProxy::didReceiveWebPageProxyMessage (this=0xaf702068, messageID=...,
I would suggest to add image null check: static QCursor* createCustomCursor(Image* image, const IntPoint& hotSpot) { + if (image->isNull()) + return NULL;
Comment on attachment 101474 [details] Indicate the presence of a null image while encoding Clearing flags on attachment: 101474 Committed r92108: <http://trac.webkit.org/changeset/92108>
All reviewed patches have been landed. Closing bug.
This seems to have been rolled out in r92113.
I think the problem here is that nullImage() function is not part of the export's file. I will add this and put up a new patch. (In reply to comment #16) > This seems to have been rolled out in r92113.
Created attachment 102631 [details] Add Image::nullImage() into exports to fix compilation on SL
Created attachment 102635 [details] Add Image::nullImage() into exports to fix compilation on SL
Comment on attachment 102635 [details] Add Image::nullImage() into exports to fix compilation on SL View in context: https://bugs.webkit.org/attachment.cgi?id=102635&action=review > Source/WebKit2/ChangeLog:11 > + * Shared/WebCoreArgumentCoders.cpp: > + (CoreIPC::::encode): > + (CoreIPC::::decode): > + In your previous patch you had filled these in. Why remove them?
Created attachment 102637 [details] Add comments in ChangeLog
(In reply to comment #20) > (From update of attachment 102635 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102635&action=review > > > Source/WebKit2/ChangeLog:11 > > + * Shared/WebCoreArgumentCoders.cpp: > > + (CoreIPC::::encode): > > + (CoreIPC::::decode): > > + > > In your previous patch you had filled these in. Why remove them? Done. Added these back.
Comment on attachment 102637 [details] Add comments in ChangeLog Seems reasonable.
Comment on attachment 102637 [details] Add comments in ChangeLog Clearing flags on attachment: 102637 Committed r92183: <http://trac.webkit.org/changeset/92183>
*** Bug 65029 has been marked as a duplicate of this bug. ***