Bug 113285 – document.createTouch crashes when document has no frame.

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebCore Misc.

[Takashi Sakamoto]

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

[Takashi Sakamoto]

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.

[Miyoung Shin]

Created attachment 229215 [details]
Patch

[Antonio Gomes]

Comment on attachment 229215 [details]
Patch

LayoutTest?

[Miyoung Shin]

(In reply to comment #3)
> (From update of attachment 229215 [details])
> LayoutTest?

Ok. I got it

[Miyoung Shin]

Created attachment 229226 [details]
Patch

[Benjamin Poulain]

Comment on attachment 229226 [details]
Patch

Please generalize the code from mouse events instead.

[Brian Burg]

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")

[Miyoung Shin]

Created attachment 229646 [details]
Patch

[WebKit Commit Bot]

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.

[Miyoung Shin]

(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.

[Miyoung Shin]

(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

[Darin Adler]

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.

[Miyoung Shin]

Created attachment 229831 [details]
Patch

[WebKit Commit Bot]

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.

[Miyoung Shin]

Created attachment 229833 [details]
Patch

[Ahmad Saleem]

Based on MDN - https://developer.mozilla.org/en-US/docs/Web/API/Document/createTouch

It is deprecated standard and is not supported by browsers (e.g. referring to compatibility table). Although, I am able to see some reference to "createTouch" in Webkit Github source code mirror in following files:

Source/WebCore/dom/DocumentTouch.h
Source/WebCore/dom/DocumentTouch.cpp

Further, I am not able to run "repo.html" test and it shows error in console but using "patch" and getting layout test out of it (Changed into below JSFiddle), it shows "PASSED".

Link - https://jsfiddle.net/sj9pc2m3/

Is something needed here or considering the test now pass from this patch reflects that it was fixed along the way? Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/95609522>

[Ryosuke Niwa]

We don't crash with the attached test case anymore.