Bug 37715 – [chromium] crash when dragging images

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Evan Stade]

code.google.com/p/chromium/issues/detail?id=41632

[Evan Stade]

Created attachment 53542 [details]
try1

first time I uploaded a patch using git. Bet it won't apply.

[Evan Martin]

Needs a test or a discussion of why it can't be tested in the changelog.  :\

[David Levin]

Comment on attachment 53542 [details]
try1

What Mr Martin said. Other than that it looks fine.

[Evan Stade]

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.

[Evan Stade]

Created attachment 53589 [details]
test

unit test added

[WebKit Review Bot]

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.

[Evan Martin]

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.

[Evan Stade]

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.

[WebKit Review Bot]

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.

[Evan Stade]

Created attachment 53697 [details]
style fix2

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 53697 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1608607

[Evan Stade]

Created attachment 53706 [details]
fix compile

[WebKit Review Bot]

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.

[David Levin]

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.

[Evan Stade]

Created attachment 53715 [details]
style, again

done

[WebKit Review Bot]

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.

[David Levin]

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.

[David Levin]

Comment on attachment 53715 [details]
style, again

Changing back to r? to allow ews to pick it up.

[WebKit Commit Bot]

Comment on attachment 53715 [details]
style, again

Clearing flags on attachment: 53715

Committed r57888: <http://trac.webkit.org/changeset/57888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.