Bug 61314

Summary: [Qt] QtWebKit crashes when dragging not loaded images
Product: WebKit Reporter: Igor Trindade Oliveira <igor.oliveira>
Component: WebKit QtAssignee: Igor Trindade Oliveira <igor.oliveira>
Severity: Critical CC: ademar, benjamin, commit-queue, diegohcg, eric, pnormand, simon.fraser, tonikitoo, webkit.review.bot
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Test Case
kling: review-
Patch none

Description Igor Trindade Oliveira 2011-05-23 15:21:04 PDT
QtWebKit crashes when dragging not loaded images.
Comment 1 Igor Trindade Oliveira 2011-05-23 15:22:17 PDT
Created attachment 94500 [details]
Test Case

To reproduce the bug try to drag the image when the page is loading.
Comment 2 Igor Trindade Oliveira 2011-05-23 15:27:27 PDT
Created attachment 94502 [details]

Proposed patch
Comment 3 WebKit Review Bot 2011-05-23 15:32:23 PDT
Attachment 94502 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/qt/DragImageQt.cpp:66:  Declaration has space between type name and * in QPixmap *dragImage  [whitespace/declaration] [3]
Total errors found: 1 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Igor Trindade Oliveira 2011-05-23 15:43:50 PDT
Created attachment 94507 [details]

Proposed patch.
Comment 5 Alexis Menard (darktears) 2011-05-23 16:10:51 PDT
Comment on attachment 94507 [details]

Looks good but can you do a layout test? I know drag and drop are tricky but is there any existing drag and drop tests you could reuse?
Comment 6 Andreas Kling 2011-05-24 04:30:09 PDT
Comment on attachment 94507 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=94507&action=review

Good catch! Needs a test though.

> Source/WebCore/platform/qt/DragImageQt.cpp:70
> -    if (!image)
> -        return 0;
> +    QPixmap* dragImage = 0;
> +    if (image && image->nativeImageForCurrentFrame())
> +        dragImage = new QPixmap(*image->nativeImageForCurrentFrame());
> -    return new QPixmap(*image->nativeImageForCurrentFrame());
> +    return dragImage;

This would read better as:

if (!image || !image->nativeImageForCurrentFrame())
    return 0;
return new QPixmap(*image->nativeImageForCurrentFrame());
Comment 7 Igor Trindade Oliveira 2011-05-24 15:19:34 PDT
Created attachment 94702 [details]

Proposed patch. Add test.
Comment 8 Alexis Menard (darktears) 2011-05-25 07:01:10 PDT
Comment on attachment 94702 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=94702&action=review

> LayoutTests/ChangeLog:8
> +        Checks if WebKit crashes when dragging a not loaded image.

dragging an image not yet loaded. better no?
Comment 9 Igor Trindade Oliveira 2011-05-25 07:05:40 PDT
Created attachment 94778 [details]

Proposed patch.
Comment 10 WebKit Commit Bot 2011-05-25 09:34:09 PDT
Comment on attachment 94778 [details]

Clearing flags on attachment: 94778

Committed r87298: <http://trac.webkit.org/changeset/87298>
Comment 11 WebKit Commit Bot 2011-05-25 09:34:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Fraser (smfr) 2011-05-25 20:33:22 PDT
This test is crashing on Mac too. Seems like other platforms need a similar fix.
Comment 13 Simon Fraser (smfr) 2011-05-25 20:34:20 PDT
I filed bug 61499.
Comment 14 Antonio Gomes 2011-05-25 20:57:27 PDT
It was failing on Qt and no bot email/bug_comment was added to this bug, as it usually happen. Failing on Mac too (according to Simon and nothing yet from the emailer bot). Maybe there is a problem with them?
Comment 15 Philippe Normand 2011-05-26 02:30:15 PDT
Filed bug 61513 for GTK
Comment 16 Igor Trindade Oliveira 2011-05-26 03:07:21 PDT
After http://trac.webkit.org/changeset/87366 , mac is not failing anymore.
Comment 17 Ademar Reis 2011-05-26 08:56:47 PDT
Revision r87298 cherry-picked into qtwebkit-2.2 with commit 8cfca23 <http://gitorious.org/webkit/qtwebkit/commit/8cfca23>