Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers
authordino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Oct 2014 20:59:32 +0000 (20:59 +0000)
committerdino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Oct 2014 20:59:32 +0000 (20:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137676
<rdar://problem/15735049>

Reviewed by Alexey Proskuryakov.

Remove the code I added that tracks the timestamp of the most recent
user gesture from the event handling dispatch, as it was both
a silly place to do it and it originally crashed when events were fired from
Worker threads (although this was fixed in r152238).

It's now recorded by going through UserGestureIndicator, which is good because
it knows when a user has triggered an event. Its constructor now takes
a pointer to Document, and updates the timestamp there if necessary.

Not all UserGestureIndicator instances needed to reset the timestamp; Those did
not have to pass along the Document.

This is untestable due to the fix mentioned above.

* WebCore.exp.in: Change constructor signature.

* accessibility/AccessibilityNodeObject.cpp: Pass a pointer to the Document into the UserGestureIndicator.
(WebCore::AccessibilityNodeObject::increment):
(WebCore::AccessibilityNodeObject::decrement):
* accessibility/AccessibilityObject.cpp: Ditto.
(WebCore::AccessibilityObject::press):

* dom/Document.cpp:
(WebCore::Document::updateLastHandledUserGestureTimestamp): Renamed.
* dom/Document.h:

* dom/EventTarget.cpp: Remove the code to update the timestamp.
(WebCore::EventTarget::fireEventListeners):

* dom/UserGestureIndicator.cpp:
(WebCore::UserGestureIndicator::UserGestureIndicator): If there is a Document and
this is a user gesture, then reset the timestamp.
* dom/UserGestureIndicator.h:

* page/EventHandler.cpp: Pass a pointer to the Document.
(WebCore::EventHandler::handleMousePressEvent):
(WebCore::EventHandler::handleMouseDoubleClickEvent):
(WebCore::EventHandler::handleMouseReleaseEvent):
(WebCore::EventHandler::keyEvent):
(WebCore::EventHandler::handleTouchEvent):

* rendering/HitTestResult.cpp: Ditto.
(WebCore::HitTestResult::toggleMediaFullscreenState):
(WebCore::HitTestResult::enterFullscreenForVideo):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@174700 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/accessibility/AccessibilityNodeObject.cpp
Source/WebCore/accessibility/AccessibilityObject.cpp
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/EventTarget.cpp
Source/WebCore/dom/UserGestureIndicator.cpp
Source/WebCore/dom/UserGestureIndicator.h
Source/WebCore/page/EventHandler.cpp
Source/WebCore/rendering/HitTestResult.cpp

index 956b531..e605da4 100644 (file)
@@ -1,3 +1,56 @@
+2014-10-14  Dean Jackson  <dino@apple.com>
+
+        Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers
+        https://bugs.webkit.org/show_bug.cgi?id=137676
+        <rdar://problem/15735049>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Remove the code I added that tracks the timestamp of the most recent
+        user gesture from the event handling dispatch, as it was both
+        a silly place to do it and it originally crashed when events were fired from
+        Worker threads (although this was fixed in r152238).
+
+        It's now recorded by going through UserGestureIndicator, which is good because
+        it knows when a user has triggered an event. Its constructor now takes
+        a pointer to Document, and updates the timestamp there if necessary.
+
+        Not all UserGestureIndicator instances needed to reset the timestamp; Those did
+        not have to pass along the Document.
+
+        This is untestable due to the fix mentioned above.
+
+        * WebCore.exp.in: Change constructor signature.
+
+        * accessibility/AccessibilityNodeObject.cpp: Pass a pointer to the Document into the UserGestureIndicator.
+        (WebCore::AccessibilityNodeObject::increment):
+        (WebCore::AccessibilityNodeObject::decrement):
+        * accessibility/AccessibilityObject.cpp: Ditto.
+        (WebCore::AccessibilityObject::press):
+
+        * dom/Document.cpp:
+        (WebCore::Document::updateLastHandledUserGestureTimestamp): Renamed.
+        * dom/Document.h:
+
+        * dom/EventTarget.cpp: Remove the code to update the timestamp.
+        (WebCore::EventTarget::fireEventListeners):
+
+        * dom/UserGestureIndicator.cpp:
+        (WebCore::UserGestureIndicator::UserGestureIndicator): If there is a Document and
+        this is a user gesture, then reset the timestamp.
+        * dom/UserGestureIndicator.h:
+
+        * page/EventHandler.cpp: Pass a pointer to the Document.
+        (WebCore::EventHandler::handleMousePressEvent):
+        (WebCore::EventHandler::handleMouseDoubleClickEvent):
+        (WebCore::EventHandler::handleMouseReleaseEvent):
+        (WebCore::EventHandler::keyEvent):
+        (WebCore::EventHandler::handleTouchEvent):
+
+        * rendering/HitTestResult.cpp: Ditto.
+        (WebCore::HitTestResult::toggleMediaFullscreenState):
+        (WebCore::HitTestResult::enterFullscreenForVideo):
+
 2014-10-14  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Unreviewed gardening. Ignore Visual Studio *.sdf files.
index 51b4c6c..d154131 100644 (file)
@@ -890,7 +890,7 @@ __ZN7WebCore20ResourceResponseBaseC2Ev
 __ZN7WebCore20TransformationMatrix5scaleEd
 __ZN7WebCore20TransformationMatrix9translateEdd
 __ZN7WebCore20UserGestureIndicator7s_stateE
-__ZN7WebCore20UserGestureIndicatorC1ENS_26ProcessingUserGestureStateE
+__ZN7WebCore20UserGestureIndicatorC1ENS_26ProcessingUserGestureStateEPNS_8DocumentE
 __ZN7WebCore20UserGestureIndicatorD1Ev
 __ZN7WebCore20deleteEmptyDirectoryERKN3WTF6StringE
 __ZN7WebCore20looksLikeAbsoluteURLEP8NSString
index 0937c40..a8a4e6b 100644 (file)
@@ -1076,13 +1076,13 @@ void AccessibilityNodeObject::alterSliderValue(bool increase)
     
 void AccessibilityNodeObject::increment()
 {
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, document());
     alterSliderValue(true);
 }
 
 void AccessibilityNodeObject::decrement()
 {
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, document());
     alterSliderValue(false);
 }
 
index 2b1919a..e47ba94 100644 (file)
@@ -845,7 +845,8 @@ bool AccessibilityObject::press()
     // Hit test at this location to determine if there is a sub-node element that should act
     // as the target of the action.
     Element* hitTestElement = nullptr;
-    if (Document* document = this->document()) {
+    Document* document = this->document();
+    if (document) {
         HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AccessibilityHitTest);
         HitTestResult hitTestResult(clickPoint());
         document->renderView()->hitTest(request, hitTestResult);
@@ -866,7 +867,7 @@ bool AccessibilityObject::press()
     if (hitTestElement && hitTestElement->isDescendantOf(pressElement))
         pressElement = hitTestElement;
     
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, document);
     pressElement->accessKeyAction(true);
     return true;
 }
index 937e2d9..2f183c3 100644 (file)
@@ -5911,8 +5911,7 @@ void Document::didRemoveEventTargetNode(Node* handler)
     }
 }
 #endif
-
-void Document::resetLastHandledUserGestureTimestamp()
+void Document::updateLastHandledUserGestureTimestamp()
 {
     m_lastHandledUserGestureTimestamp = monotonicallyIncreasingTime();
 }
index a517d3b..c1102bb 100644 (file)
@@ -1202,7 +1202,7 @@ public:
     WEBCORE_EXPORT void didRemoveWheelEventHandler();
 
     double lastHandledUserGestureTimestamp() const { return m_lastHandledUserGestureTimestamp; }
-    void resetLastHandledUserGestureTimestamp();
+    void updateLastHandledUserGestureTimestamp();
 
 #if ENABLE(TOUCH_EVENTS)
     bool hasTouchEventHandlers() const { return (m_touchEventTargets.get()) ? m_touchEventTargets->size() : false; }
index 989f502..8462fed 100644 (file)
@@ -213,7 +213,6 @@ void EventTarget::fireEventListeners(Event* event, EventTargetData* d, EventList
     // Also, don't fire event listeners added during event dispatch. Conveniently, all new event listeners will be added
     // after or at index |size|, so iterating up to (but not including) |size| naturally excludes new event listeners.
 
-    bool userEventWasHandled = false;
     size_t i = 0;
     size_t size = entry.size();
     if (!d->firingEventIterators)
@@ -244,13 +243,9 @@ void EventTarget::fireEventListeners(Event* event, EventTargetData* d, EventList
         // To match Mozilla, the AT_TARGET phase fires both capturing and bubbling
         // event listeners, even though that violates some versions of the DOM spec.
         registeredListener.listener->handleEvent(context, event);
-        if (!userEventWasHandled && ScriptController::processingUserGesture())
-            userEventWasHandled = true;
         InspectorInstrumentation::didHandleEvent(cookie);
     }
     d->firingEventIterators->removeLast();
-    if (userEventWasHandled && document)
-        document->resetLastHandledUserGestureTimestamp();
 
     if (document)
         InspectorInstrumentation::didDispatchEvent(willDispatchEventCookie);
index 5fd20b6..15134e3 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "UserGestureIndicator.h"
 
+#include "Document.h"
 #include <wtf/MainThread.h>
 
 namespace WebCore {
@@ -37,7 +38,7 @@ static bool isDefinite(ProcessingUserGestureState state)
 
 ProcessingUserGestureState UserGestureIndicator::s_state = DefinitelyNotProcessingUserGesture;
 
-UserGestureIndicator::UserGestureIndicator(ProcessingUserGestureState state)
+UserGestureIndicator::UserGestureIndicator(ProcessingUserGestureState state, Document* document)
     : m_previousState(s_state)
 {
     // Silently ignore UserGestureIndicators on non main threads.
@@ -47,6 +48,9 @@ UserGestureIndicator::UserGestureIndicator(ProcessingUserGestureState state)
     if (isDefinite(state))
         s_state = state;
     ASSERT(isDefinite(s_state));
+
+    if (document && s_state == DefinitelyProcessingUserGesture)
+        document->topDocument().updateLastHandledUserGestureTimestamp();
 }
 
 UserGestureIndicator::~UserGestureIndicator()
index 355ca69..2dc7a89 100644 (file)
@@ -30,6 +30,8 @@
 
 namespace WebCore {
 
+class Document;
+
 enum ProcessingUserGestureState {
     DefinitelyProcessingUserGesture,
     PossiblyProcessingUserGesture,
@@ -41,10 +43,10 @@ class UserGestureIndicator {
 public:
     static bool processingUserGesture();
 
-    WEBCORE_EXPORT explicit UserGestureIndicator(ProcessingUserGestureState);
+    // If a document is provided, its last known user gesture timestamp is updated.
+    WEBCORE_EXPORT explicit UserGestureIndicator(ProcessingUserGestureState, Document* = nullptr);
     WEBCORE_EXPORT ~UserGestureIndicator();
 
-
 private:
     WEBCORE_EXPORT static ProcessingUserGestureState s_state;
     ProcessingUserGestureState m_previousState;
index 661ea59..9958a03 100644 (file)
@@ -1653,7 +1653,7 @@ bool EventHandler::handleMousePressEvent(const PlatformMouseEvent& platformMouse
         return true;
 #endif
 
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, m_frame.document());
 
     // FIXME (bug 68185): this call should be made at another abstraction layer
     m_frame.loader().resetMultipleFormSubmissionProtection();
@@ -1790,7 +1790,7 @@ bool EventHandler::handleMouseDoubleClickEvent(const PlatformMouseEvent& platfor
 
     m_frame.selection().setCaretBlinkingSuspended(false);
 
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, m_frame.document());
 
     // We get this instead of a second mouse-up 
     m_mousePressed = false;
@@ -2035,7 +2035,7 @@ bool EventHandler::handleMouseReleaseEvent(const PlatformMouseEvent& platformMou
         return true;
 #endif
 
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, m_frame.document());
 
 #if ENABLE(PAN_SCROLLING)
     m_autoscrollController->handleMouseReleaseEvent(platformMouseEvent);
@@ -3071,7 +3071,7 @@ bool EventHandler::keyEvent(const PlatformKeyboardEvent& initialKeyEvent)
     if (!element)
         return false;
 
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, m_frame.document());
     UserTypingGestureIndicator typingGestureIndicator(m_frame);
 
     if (FrameView* view = m_frame.view())
@@ -3799,7 +3799,7 @@ bool EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
 
     const Vector<PlatformTouchPoint>& points = event.touchPoints();
 
-    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
+    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture, m_frame.document());
 
     unsigned i;
     bool freshTouchEvents = true;
index 5a5bb4e..6ba08dd 100644 (file)
@@ -414,7 +414,7 @@ void HitTestResult::toggleMediaFullscreenState() const
 #if ENABLE(VIDEO)
     if (HTMLMediaElement* mediaElement = this->mediaElement()) {
         if (mediaElement->isVideo() && mediaElement->supportsFullscreen()) {
-            UserGestureIndicator indicator(DefinitelyProcessingUserGesture);
+            UserGestureIndicator indicator(DefinitelyProcessingUserGesture, &mediaElement->document());
             mediaElement->toggleFullscreenState();
         }
     }
@@ -428,7 +428,7 @@ void HitTestResult::enterFullscreenForVideo() const
     if (is<HTMLVideoElement>(mediaElement)) {
         HTMLVideoElement& videoElement = downcast<HTMLVideoElement>(*mediaElement);
         if (!videoElement.isFullscreen() && mediaElement->supportsFullscreen()) {
-            UserGestureIndicator indicator(DefinitelyProcessingUserGesture);
+            UserGestureIndicator indicator(DefinitelyProcessingUserGesture, &mediaElement->document());
             videoElement.enterFullscreen();
         }
     }