Teach ScriptRunner how to yield and post on loading task queue

Source/core/dom/ScriptRunner.cpp

Index: Source/core/dom/ScriptRunner.cpp
diff --git a/Source/core/dom/ScriptRunner.cpp b/Source/core/dom/ScriptRunner.cpp
index 88ed1e1c360659213e8affaf7f2ffebd37670c3f..6ac74e08d14901f0d9910ce53d7b75f391f998d6 100644
--- a/Source/core/dom/ScriptRunner.cpp
+++ b/Source/core/dom/ScriptRunner.cpp
@@ -30,12 +30,14 @@
 #include "core/dom/Element.h"
 #include "core/dom/ScriptLoader.h"
 #include "platform/heap/Handle.h"
+#include "platform/scheduler/Scheduler.h"
+#include "wtf/Functional.h"
 
 namespace blink {
 
 ScriptRunner::ScriptRunner(Document* document)
     : m_document(document)
-    , m_timer(this, &ScriptRunner::timerFired)
+    , m_executeScriptsTaskFactory(WTF::bind(&ScriptRunner::executeScripts, this))
 {
     ASSERT(document);
 }
@@ -77,13 +79,13 @@ void ScriptRunner::queueScriptForExecution(ScriptLoader* scriptLoader, Execution
 
 void ScriptRunner::suspend()
 {
-    m_timer.stop();
+    m_executeScriptsTaskFactory.cancel();
 }
 
 void ScriptRunner::resume()
 {
     if (hasPendingScripts())
-        m_timer.startOneShot(0, FROM_HERE);
+        Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task());
 }
 
 void ScriptRunner::notifyScriptReady(ScriptLoader* scriptLoader, ExecutionType executionType)
@@ -99,7 +101,8 @@ void ScriptRunner::notifyScriptReady(ScriptLoader* scriptLoader, ExecutionType e
         ASSERT(!m_scriptsToExecuteInOrder.isEmpty());
         break;
     }
-    m_timer.startOneShot(0, FROM_HERE);
+    // FIXME: Rename task() so that it's obvious it cancels any pending task.
+    Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task());

[marja, 2015/02/20 09:34:37]

Hmm, so, does this mean that if the task is already in the task queue, we take
it out and post another one which goes to the end of the task queue? Why is that
desired behavior? Shouldn't we rather not post a task if there's one in flight
already?

[alex clarke (OOO till 29th), 2015/02/20 11:59:34]

Good point, this lead to some discussion in the office.  The actual behavior of
the Blink Timer Heap is quite complicated.  In the case where it's lightly
loaded, calling m_timer.startOneShot(0, ...) at time T+0 and T+1 will cause the
underlying chromium timer to get cancelled and another task to get posted to
call the shared platform timers (i.e. identical behavior to this).   If the
Blink Timer heap is heavily loaded, it's quite likely that calling
m_timer.startOneShot(0, ...) the second time won't change the first entry in the
timer heap.

Anyway I think it's probably best to use the in-flight task if one exists
because in a (likely theoretical) scenario where a very large number of
notifyScriptReady calls happen we might get unreasonable delay.

As a side note I look forward to the day when we can delete the Blink Timer
Heap, the behavior of that code is far too difficult to reason about.

 }
 
 void ScriptRunner::notifyScriptLoadError(ScriptLoader* scriptLoader, ExecutionType executionType)
@@ -153,24 +156,35 @@ void ScriptRunner::movePendingAsyncScript(ScriptRunner* newRunner, ScriptLoader*
     }
 }
 
-void ScriptRunner::timerFired(Timer<ScriptRunner>* timer)
+void ScriptRunner::executeScripts()
 {
-    ASSERT_UNUSED(timer, timer == &m_timer);
-
     RefPtrWillBeRawPtr<Document> protect(m_document.get());
 
-    WillBeHeapVector<RawPtrWillBeMember<ScriptLoader> > scriptLoaders;
-    scriptLoaders.swap(m_scriptsToExecuteSoon);
-
-    size_t numInOrderScriptsToExecute = 0;
-    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]->isReady(); ++numInOrderScriptsToExecute)
-        scriptLoaders.append(m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]);
-    if (numInOrderScriptsToExecute)
-        m_scriptsToExecuteInOrder.remove(0, numInOrderScriptsToExecute);
+    // New scripts are always appended to m_scriptsToExecuteSoon and m_scriptsToExecuteInOrder (never prepended)
+    // so as long as we keep track of the current totals, we can ensure the order of execution if new scripts
+    // are added while executing the current ones.
+    // NOTE a yield followed by a notifyScriptReady(... ASYNC_EXECUTION) will result in that script executing
+    // before any pre-existing ScriptsToExecuteInOrder.
+    size_t numScriptsToExecuteSoon = m_scriptsToExecuteSoon.size();
+    size_t numScriptsToExecuteInOrder = m_scriptsToExecuteInOrder.size();
+    for (size_t i = 0; i < numScriptsToExecuteSoon; i++) {
+        if (Scheduler::shared()->shouldYieldForHighPriorityWork()) {

[sof, 2015/02/20 07:48:50]

Add a private helper method to avoid the repetition of these details,

  if (shouldYieldForHighPriorityWork())
      return;

[alex clarke (OOO till 29th), 2015/02/20 11:59:34]

Done.

+            Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task());
+            return;
+        }
+        m_scriptsToExecuteSoon.takeFirst()->execute();
+        m_document->decrementLoadEventDelayCount();
+    }
 
-    size_t size = scriptLoaders.size();
-    for (size_t i = 0; i < size; ++i) {
-        scriptLoaders[i]->execute();
+    for (size_t i = 0; i < numScriptsToExecuteInOrder; i++) {
+        if (!m_scriptsToExecuteInOrder.first()->isReady()) {

[sof, 2015/02/20 07:48:50]

Let's add an assert about non-emptiness, just in case.

[alex clarke (OOO till 29th), 2015/02/20 11:59:34]

Done.

+            break;
+        }

[sof, 2015/02/20 07:48:50]

Redundant braces.

[alex clarke (OOO till 29th), 2015/02/20 11:59:34]

Done.

+        if (Scheduler::shared()->shouldYieldForHighPriorityWork()) {
+            Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task());
+            return;
+        }
+        m_scriptsToExecuteInOrder.takeFirst()->execute();
         m_document->decrementLoadEventDelayCount();
     }
 }