Changeset 55816 in webkit

Timestamp:

Mar 10, 2010 4:08:18 PM (14 years ago)

Author:

dimich@chromium.org

Message:

Make Document::postTask to use a single queue of tasks, to fire them in order
​https://bugs.webkit.org/show_bug.cgi?id=35943

Reviewed by Darin Adler.

JavaScriptCore:

The patch uses CFRunLoopTimer to schedule execution of tasks instead of performSelectorOnMainThread which apparently can starve other event sources.
The timer is used when the schedule request is coming on the main thread itself. This happens when the task is posted on the main thread or
when too many tasks are posted and the queue does 'stop and re-schedule' to make sure run loop has a chance to execute other events.

• wtf/mac/MainThreadMac.mm:

(WTF::timerFired):
(WTF::postTimer):
(WTF::scheduleDispatchFunctionsOnMainThread): Use timer posted to the current RunLoop if scheduling the task execution while on the main thread.

WebCore:

Test: existing worker-cloneport.html which was broken by initial patch in ​http://trac.webkit.org/changeset/55593.
Additional test which indirectly verifies the order of execution will come as part of ​https://bugs.webkit.org/show_bug.cgi?id=34726

• dom/Document.cpp:

(WebCore::Document::postTask): Always use the same task queue, independent of what thread is posting the task.

LayoutTests:

Updated the test since the order of events coming from independent task sources has changed.
This is a behavior change, but is consistent with the spec (which specifically says these events may be fired
in any order) and the last version of FF (3.5.8) which was completing the test successfully.

• http/tests/appcache/top-frame-2-expected.txt:
• http/tests/appcache/top-frame-2.html:

Location:

trunk

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/wtf/mac/MainThreadMac.mm (modified) (2 diffs)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/appcache/top-frame-2-expected.txt (modified) (1 diff)
• LayoutTests/http/tests/appcache/top-frame-2.html (modified) (3 diffs)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/Document.cpp (modified) (2 diffs)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2010-03-10  Dmitry Titov  <dimich@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Make Document::postTask to use a single queue of tasks, to fire them in order
+        https://bugs.webkit.org/show_bug.cgi?id=35943
+
+        The patch uses CFRunLoopTimer to schedule execution of tasks instead of performSelectorOnMainThread which apparently can starve other event sources.
+        The timer is used when the schedule request is coming on the main thread itself. This happens when the task is posted on the main thread or
+        when too many tasks are posted and the queue does 'stop and re-schedule' to make sure run loop has a chance to execute other events.
+
+        * wtf/mac/MainThreadMac.mm:
+        (WTF::timerFired):
+        (WTF::postTimer):
+        (WTF::scheduleDispatchFunctionsOnMainThread): Use timer posted to the current RunLoop if scheduling the task execution while on the main thread.
+
 2010-03-10  Geoffrey Garen  <ggaren@apple.com>
 

trunk/JavaScriptCore/wtf/mac/MainThreadMac.mm

@@ -30 +30 @@
 #import "MainThread.h"
 
+#import <CoreFoundation/CoreFoundation.h>
 #import <Foundation/NSThread.h>
 #import <wtf/Assertions.h>
+#import <wtf/Threading.h>
 
 @interface WTFMainThreadCaller : NSObject {
@@ -64 +66 @@
 }
 
+static bool isTimerPosted; // This is only accessed on the 'main' thread.
+
+static void timerFired(CFRunLoopTimerRef timer, void*)
+{
+    CFRelease(timer);
+    isTimerPosted = false;
+    WTF::dispatchFunctionsFromMainThread();
+}
+
+void postTimer()
+{
+    ASSERT(isMainThread());
+
+    if (isTimerPosted)
+        return;
+
+    isTimerPosted = true;
+    CFRunLoopAddTimer(CFRunLoopGetCurrent(), CFRunLoopTimerCreate(0, 0, 0, 0, 0, timerFired, 0), kCFRunLoopCommonModes);
+}
+
+
 void scheduleDispatchFunctionsOnMainThread()
 {
     ASSERT(staticMainThreadCaller);
+
+    if (isMainThread()) {
+        postTimer();
+        return;
+    }
+
 #if USE(WEB_THREAD)
     [staticMainThreadCaller performSelector:@selector(call) onThread:webThread withObject:nil waitUntilDone:NO];

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2010-03-10  Dmitry Titov  <dimich@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Make Document::postTask to use a single queue of tasks, to fire them in order
+        https://bugs.webkit.org/show_bug.cgi?id=35943
+
+        Updated the test since the order of events coming from independent task sources has changed.
+        This is a behavior change, but is consistent with the spec (which specifically says these events may be fired
+        in any order) and the last version of FF (3.5.8) which was completing the test successfully.
+
+        * http/tests/appcache/top-frame-2-expected.txt:
+        * http/tests/appcache/top-frame-2.html:
+
 2010-03-10  Antonio Gomes  <tonikitoo@webkit.org>
 

trunk/LayoutTests/http/tests/appcache/top-frame-2-expected.txt

@@ -3 +3 @@
 Should say SUCCESS:
 
-checking
 SUCCESS
 

trunk/LayoutTests/http/tests/appcache/top-frame-2.html

@@ -17 +17 @@
 }
 
+function debug(message)
+{
+    // If running manually in the browser, print the sequence of events.
+    if (!window.layoutTestController)
+        log(message);
+}
+
+var receivedExpectedMessage = false;
+var receivedCheckingEvent = false;
+var receivedNoupdateEvent = false;
+
 function test()
 {
@@ -23 +34 @@
 
     // When a new main resource is associated with the cache, an update should be started.
-    applicationCache.onchecking = function() { log("checking") }
+    applicationCache.onchecking = function() { debug("checking"); receivedCheckingEvent = true; checkDone();  }
+    applicationCache.onnoupdate = function() { debug("noupdate"); receivedNoupdateEvent = true; checkDone();  }
 
     var ifr = document.createElement("iframe");
     ifr.setAttribute("src", "resources/subframe-2.html");
     document.body.appendChild(ifr);
+}
+
+function checkDone()
+{
+    if (receivedExpectedMessage && receivedCheckingEvent && receivedNoupdateEvent) {
+        log("SUCCESS");
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    }
 }
 
@@ -36 +57 @@
 applicationCache.onerror = function() { log("FAIL: received unexpected error event") }
 
-window.addEventListener("message", function() { log("SUCCESS"); if (window.layoutTestController) layoutTestController.notifyDone() }, false);
+window.addEventListener("message", function() { debug("message"); receivedExpectedMessage = true; checkDone(); }, false);
 
 </script>

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-03-10  Dmitry Titov  <dimich@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Make Document::postTask to use a single queue of tasks, to fire them in order
+        https://bugs.webkit.org/show_bug.cgi?id=35943
+
+        Test: existing worker-cloneport.html which was broken by initial patch in http://trac.webkit.org/changeset/55593.
+        Additional test which indirectly verifies the order of execution will come as part of https://bugs.webkit.org/show_bug.cgi?id=34726
+
+        * dom/Document.cpp:
+        (WebCore::Document::postTask): Always use the same task queue, independent of what thread is posting the task.
+
 2010-03-10  Sanjeev Radhakrishnan  <sanjeevr@chromium.org>
 

trunk/WebCore/dom/Document.cpp

@@ -4686 +4686 @@
 }
 
-class ScriptExecutionContextTaskTimer : public TimerBase {
-public:
-    ScriptExecutionContextTaskTimer(PassRefPtr<Document> context, PassOwnPtr<ScriptExecutionContext::Task> task)
-        : m_context(context)
-        , m_task(task)
-    {
-    }
-
-private:
-    virtual void fired()
-    {
-        m_task->performTask(m_context.get());
-        delete this;
-    }
-
-    RefPtr<Document> m_context;
-    OwnPtr<ScriptExecutionContext::Task> m_task;
-};
-
 struct PerformTaskContext : Noncopyable {
     PerformTaskContext(PassRefPtr<DocumentWeakReference> documentReference, PassOwnPtr<ScriptExecutionContext::Task> task)
@@ -4731 +4712 @@
 void Document::postTask(PassOwnPtr<Task> task)
 {
-    if (isMainThread()) {
-        ScriptExecutionContextTaskTimer* timer = new ScriptExecutionContextTaskTimer(static_cast<Document*>(this), task);
-        timer->startOneShot(0);
-    } else {
-        callOnMainThread(performTask, new PerformTaskContext(m_weakReference, task));
-    }
+    callOnMainThread(performTask, new PerformTaskContext(m_weakReference, task));
 }