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 956b5314a7e12993582928a4d771198d92171dd0..e605da47f7c95b1a33df84ff95c331b0d031d04f 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 51b4c6c3fc2649fab17bc291373e6779aa99fa53..d154131d2cf1b220070b411316305381c6714f9f 100644 (file)
@@ -890,7 +890,7 @@ __ZN7WebCore20ResourceResponseBaseC2Ev
 __ZN7WebCore20TransformationMatrix5scaleEd
 __ZN7WebCore20TransformationMatrix9translateEdd
 __ZN7WebCore20UserGestureIndicator7s_stateE
-__ZN7WebCore20UserGestureIndicatorC1ENS_26ProcessingUserGestureStateE
+__ZN7WebCore20UserGestureIndicatorC1ENS_26ProcessingUserGestureStateEPNS_8DocumentE
 __ZN7WebCore20UserGestureIndicatorD1Ev
 __ZN7WebCore20deleteEmptyDirectoryERKN3WTF6StringE
 __ZN7WebCore20looksLikeAbsoluteURLEP8NSString
index 0937c40a83d36789b48c51cf72c3d401304ce972..a8a4e6b37d2ef8a6d80ab2a3b70888814ba9d769 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 2b1919a6b5c3505d3038d2b7693a72b2dd8e272e..e47ba9498f30a69fc9fed7f9fc6f31b0f9e1b023 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 937e2d92e64e2d2ed435e19b914b588be805995c..2f183c3794d795fb3e25a00adf7e894da1226302 100644 (file)
@@ -5911,8 +5911,7 @@ void Document::didRemoveEventTargetNode(Node* handler)
     }
 }
 #endif
-
-void Document::resetLastHandledUserGestureTimestamp()
+void Document::updateLastHandledUserGestureTimestamp()
 {
     m_lastHandledUserGestureTimestamp = monotonicallyIncreasingTime();
 }
index a517d3b5f7b677946edc9ea24a9a76d21a9b1e72..c1102bb35d9fb5163e196f56630ddb1bd1a31706 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 989f50261f6151f9c6316e0f968a4f87c094ae2f..8462fed207960718ca548b109ed9a7f5280b9836 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 5fd20b6c77ff28d3e3f86e7fe2290aeacfc96838..15134e3390ae537ffd4143b225ddee1160cb8e74 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 355ca69d29203d2fff6e553c94009debb8c48cd2..2dc7a895e79da9e1d81a34382d3d74bdc53ea36f 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 661ea59f0010712c8763469c6f933a9e84b2f4a8..9958a03c285dd0922f4c6c883ffa1d843a75598a 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 5a5bb4e8faf370d1376eb601100029813ed8300f..6ba08dd2f8bbc3af664041a4ec1d5b12ef4ac0c5 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();
         }
     }