Bug 37715 - [chromium] crash when dragging images
Summary: [chromium] crash when dragging images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Evan Stade
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-16 11:11 PDT by Evan Stade
Modified: 2010-04-20 06:44 PDT (History)
4 users (show)

See Also:


Attachments
try1 (1.32 KB, patch)
2010-04-16 11:16 PDT, Evan Stade
levin: review-
Details | Formatted Diff | Diff
test (7.38 KB, patch)
2010-04-16 20:15 PDT, Evan Stade
no flags Details | Formatted Diff | Diff
style fixes (7.38 KB, patch)
2010-04-19 11:40 PDT, Evan Stade
no flags Details | Formatted Diff | Diff
style fix2 (7.39 KB, patch)
2010-04-19 11:43 PDT, Evan Stade
no flags Details | Formatted Diff | Diff
fix compile (deleted)
2010-04-19 12:41 PDT, Evan Stade
levin: review-
Details | Formatted Diff | Diff
style, again (7.38 KB, patch)
2010-04-19 13:37 PDT, Evan Stade
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2010-04-16 11:11:45 PDT
code.google.com/p/chromium/issues/detail?id=41632
Comment 1 Evan Stade 2010-04-16 11:16:45 PDT
Created attachment 53542 [details]
try1

first time I uploaded a patch using git. Bet it won't apply.
Comment 2 Evan Martin 2010-04-16 11:31:18 PDT
Needs a test or a discussion of why it can't be tested in the changelog.  :\
Comment 3 David Levin 2010-04-16 11:49:03 PDT
Comment on attachment 53542 [details]
try1

What Mr Martin said. Other than that it looks fine.
Comment 4 Evan Stade 2010-04-16 16:03:29 PDT
ok, I found a repro. Go to a page with an SVG, and then hit shift-f5 and drag like mad from the point where the SVG is going to load. After several tries it should crash the renderer. Unfortunately it appears my js abilities are too weak to create a test out of this.
Comment 5 Evan Stade 2010-04-16 20:15:26 PDT
Created attachment 53589 [details]
test

unit test added
Comment 6 WebKit Review Bot 2010-04-16 20:17:06 PDT
Attachment 53589 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/DragImageTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/tests/DragImageTest.cpp:94:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/tests/DragImageTest.cpp:130:  Use 0 instead of NULL.  [readability/null] [4]
WebKit/chromium/tests/DragImageTest.cpp:136:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Evan Martin 2010-04-17 14:29:07 PDT
Whoa, we can do unit tests in webkit?

Probably want s/m_NativeImage/m_nativeImage/g.

The code looks good to me, but I am not a reviewer.
Comment 8 Evan Stade 2010-04-19 11:40:51 PDT
Created attachment 53696 [details]
style fixes

Yes, the unit tests are a new development I believe. (git log says Jan 8 -- not that new apparently)

I believe that the header style error is not applicable for unit tests.
Comment 9 WebKit Review Bot 2010-04-19 11:41:40 PDT
Attachment 53696 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/DragImageTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/tests/DragImageTest.cpp:94:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/tests/DragImageTest.cpp:130:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Evan Stade 2010-04-19 11:43:41 PDT
Created attachment 53697 [details]
style fix2
Comment 11 WebKit Review Bot 2010-04-19 11:48:06 PDT
Attachment 53697 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/DragImageTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Review Bot 2010-04-19 12:34:05 PDT
Attachment 53697 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1608607
Comment 13 Evan Stade 2010-04-19 12:41:18 PDT
Created attachment 53706 [details]
fix compile
Comment 14 WebKit Review Bot 2010-04-19 12:48:22 PDT
Attachment 53706 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/DragImageTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 David Levin 2010-04-19 13:33:41 PDT
Comment on attachment 53706 [details]
fix compile


> diff --git a/WebKit/chromium/tests/DragImageTest.cpp b/WebKit/chromium/tests/DragImageTest.cpp
> +class TestImage : public Image {
> +public:
> +
> +    explicit TestImage(const IntSize& size)
> +        : Image(0),
> +          m_Size(size)

Two nits: m_size (instead of m_Size) and put the comma before the m_size instead of on the previous line.

That's all though.
Comment 16 Evan Stade 2010-04-19 13:37:07 PDT
Created attachment 53715 [details]
style, again

done
Comment 17 WebKit Review Bot 2010-04-19 13:43:30 PDT
Attachment 53715 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/DragImageTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/tests/DragImageTest.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 David Levin 2010-04-19 13:47:01 PDT
Comment on attachment 53715 [details]
style, again

Didn't mark cq+ yet... as it would be nice to wait for the chromium ews to turn green.
Comment 19 David Levin 2010-04-19 13:58:14 PDT
Comment on attachment 53715 [details]
style, again

Changing back to r? to allow ews to pick it up.
Comment 20 WebKit Commit Bot 2010-04-20 06:44:40 PDT
Comment on attachment 53715 [details]
style, again

Clearing flags on attachment: 53715

Committed r57888: <http://trac.webkit.org/changeset/57888>
Comment 21 WebKit Commit Bot 2010-04-20 06:44:46 PDT
All reviewed patches have been landed.  Closing bug.