Bug 113285 - document.createTouch crashes when document has no frame.
Summary: document.createTouch crashes when document has no frame.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-26 02:23 PDT by Takashi Sakamoto
Modified: 2016-03-09 09:39 PST (History)
9 users (show)

See Also:


Attachments
repro.html (292 bytes, text/html)
2013-03-26 02:23 PDT, Takashi Sakamoto
no flags Details
Patch (1.47 KB, patch)
2014-04-12 18:46 PDT, Miyoung Shin
no flags Details | Formatted Diff | Diff
Patch (3.77 KB, patch)
2014-04-13 02:03 PDT, Miyoung Shin
no flags Details | Formatted Diff | Diff
Patch (23.47 KB, patch)
2014-04-18 08:39 PDT, Miyoung Shin
no flags Details | Formatted Diff | Diff
Patch (20.28 KB, patch)
2014-04-21 14:30 PDT, Miyoung Shin
no flags Details | Formatted Diff | Diff
Patch (20.28 KB, patch)
2014-04-21 14:38 PDT, Miyoung Shin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2013-03-26 02:23:12 PDT
Created attachment 195039 [details]
repro.html

reported by fuzzer:

Crash type	UNKNOWN
Crash address	0x000000000b00
Crash state	- crash stack -
WebCore::Touch::Touch
WebCore::Document::createTouch
WebCore::DocumentV8Internal::createTouchMethodCallback

    #0 0x551f47 in WebCore::Frame::pageZoomFactor() const third_party/WebKit/Source/WebCore/page/Frame.h:155
    #1 0xadd78a in WebCore::Touch::Touch(WebCore::Frame*, WebCore::EventTarget*, unsigned int, int, int, int, int, int, int, float, float) third_party/WebKit/Source/WebCore/dom/Touch.cpp:72
    #2 0x9cdc59 in WebCore::Touch::create(WebCore::Frame*, WebCore::EventTarget*, unsigned int, int, int, int, int, int, int, float, float) third_party/WebKit/Source/WebCore/dom/Touch.h:47
    #3 0x9cdbbf in WebCore::Document::createTouch(WebCore::DOMWindow*, WebCore::EventTarget*, int, int, int, int, int, int, int, float, float, int&) const third_party/WebKit/Source/WebCore/dom/Document.cpp:5609
    #4 0x2820762 in WebCore::DocumentV8Internal::createTouchMethod(v8::Arguments const&) out/Release/obj/gen/webcore/bindings/V8Document.cpp:2648
    #5 0xd56ca3 in v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<false>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) v8/src/builtins.cc:1327

https://cluster-fuzz.appspot.com/testcase?key=171689017
Comment 1 Takashi Sakamoto 2013-03-26 02:38:42 PDT
This bug is caused by change r122286 (https://bugs.webkit.org/show_bug.cgi?id=88807).

Source/WebCore/dom/Touch.cpp:
@@Touch::Touch(Frame* frame, EventTarget* target, unsigned identifier, int screenX
    , m_rotationAngle(rotationAngle)
    , m_force(force)
{
+   float scaleFactor = frame->pageZoomFactor() * frame->frameScaleFactor();
+   float x = pageX * scaleFactor;
+   float y = pageY * scaleFactor;
+   m_absoluteLocation = roundedLayoutPoint(FloatPoint(x, y));
}

Need to check whether frame is available or not.
Comment 2 Miyoung Shin 2014-04-12 18:46:53 PDT
Created attachment 229215 [details]
Patch
Comment 3 Antonio Gomes 2014-04-12 18:52:30 PDT
Comment on attachment 229215 [details]
Patch

LayoutTest?
Comment 4 Miyoung Shin 2014-04-12 19:20:43 PDT
(In reply to comment #3)
> (From update of attachment 229215 [details])
> LayoutTest?

Ok. I got it
Comment 5 Miyoung Shin 2014-04-13 02:03:27 PDT
Created attachment 229226 [details]
Patch
Comment 6 Benjamin Poulain 2014-04-14 13:33:28 PDT
Comment on attachment 229226 [details]
Patch

Please generalize the code from mouse events instead.
Comment 7 Brian Burg 2014-04-14 17:02:48 PDT
Comment on attachment 229226 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Because it is possible that the Document has no the Frame.

"Because it is..." isn't necessary, you can delete that line.

> LayoutTests/fast/events/touch/document-create-touch-new-document.html:15
> +<p id="description"></p>

You may want to state the situation being tested here (i.e., "Should not crash when creating touch event in document without a frame")
Comment 8 Miyoung Shin 2014-04-18 08:39:24 PDT
Created attachment 229646 [details]
Patch
Comment 9 WebKit Commit Bot 2014-04-18 08:40:09 PDT
Attachment 229646 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:47:  Wrong number of spaces before statement. (expected: 41)  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Miyoung Shin 2014-04-18 08:43:18 PDT
(In reply to comment #6)
> (From update of attachment 229226 [details])
> Please generalize the code from mouse events instead.

Hi, I've uploaded new patch.
There is the style checking error, but I'm not sure whether I should fix it or not because It's already existed.
I hope to get your feedback.
Thanks.
Comment 11 Miyoung Shin 2014-04-18 08:44:44 PDT
(In reply to comment #7)
> (From update of attachment 229226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229226&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Because it is possible that the Document has no the Frame.
> 
> "Because it is..." isn't necessary, you can delete that line.
> 
> > LayoutTests/fast/events/touch/document-create-touch-new-document.html:15
> > +<p id="description"></p>
> 
> You may want to state the situation being tested here (i.e., "Should not crash when creating touch event in document without a frame")

==> I've uloaded new patch to change some test code regarding to your comment
Comment 12 Darin Adler 2014-04-19 14:37:48 PDT
Comment on attachment 229646 [details]
Patch

It is *not* good to store a Frame* in every event, which is what this patch does. This looks like overkill. These helper functions don’t need to be put into a class, and we really don’t want to get multiple inheritance and virtual functions in the event classes just to do a bit more code sharing.
Comment 13 Miyoung Shin 2014-04-21 14:30:10 PDT
Created attachment 229831 [details]
Patch
Comment 14 WebKit Commit Bot 2014-04-21 14:31:27 PDT
Attachment 229831 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ComputeContent.h:31:  #ifndef header guard has wrong style, please use: ComputeContent_h  [build/header_guard] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Miyoung Shin 2014-04-21 14:38:59 PDT
Created attachment 229833 [details]
Patch