Reland "Correctly set ScriptState in the image loader microtask"

Source/core/loader/ImageLoader.cpp

Index: Source/core/loader/ImageLoader.cpp
diff --git a/Source/core/loader/ImageLoader.cpp b/Source/core/loader/ImageLoader.cpp
index 8e1adb8b22220c11f5fdf34f0fcf5e54bd36932e..df1302f918b82872e9f35dbdaad6e91386ea548d 100644
--- a/Source/core/loader/ImageLoader.cpp
+++ b/Source/core/loader/ImageLoader.cpp
@@ -23,6 +23,9 @@
 #include "core/loader/ImageLoader.h"
 
 #include "bindings/core/v8/ScriptController.h"
+#include "bindings/core/v8/ScriptState.h"
+#include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/V8PerIsolateData.h"
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
 #include "core/dom/IncrementLoadEventDelayCount.h"
@@ -85,9 +88,20 @@ public:
         , m_updateBehavior(updateBehavior)
         , m_weakFactory(this)
     {
+        v8::Isolate* isolate = V8PerIsolateData::mainThreadIsolate();
+        v8::HandleScope scope(isolate);

[haraken, 2015/05/28 11:37:50]

Nit: I think we should have the handle scope in hasCurrentScriptState.

+        if (ScriptState::hasCurrentScriptState(isolate)) {
+            m_scriptState = ScriptState::current(isolate);
+        } else {
+            m_scriptState = ScriptState::from(toV8Context(&loader->element()->document(), DOMWrapperWorld::mainWorld()));

[haraken, 2015/05/28 10:57:38]

Why can't we always use the line 96?

ScriptState::hasCurrentScriptState(isolate) is a hacky API whose usage should be
limited. "if(ScriptState::hasCurrentScriptState(isolate))" means that "if the
debugger context is not on top of the context stack".

So what we're doing here is:

  if (debugger context is not on top of the context stack)
    use ScriptState::current();
  else
    use ScriptState::from(toV8Context());

Here are two problems:

- ScriptState::current() is not guaranteed to be the same as
ScriptState::from(toV8Context()). I think the correct ScriptState we should use
is ScriptState::from(toV8Context()).

- The debugger context shouldn't be on top of the context stack when you reach
here. (It can rarely happen.)

So, in short, my suggestion is to unconditionally use
ScriptState::from(toV8Context()) here.

[jochen (gone - plz use gerrit), 2015/05/28 11:16:49]

there might be no context at all, so I need two cases

[haraken, 2015/05/28 11:37:50]

OK, understood.

If JS creates a micro task, we should run the micro task in the context of the
JS.

If DOM creates a micro task, there can be no associated context at this point.
Since this can happen only in the main world, we should run the micro task in
the context associated with the document and the main world.

Maybe we can add a comment.

+        }
+    }
+
+    ~Task() override
+    {
     }
 
-    virtual void run() override
+    void run() override
     {
         if (m_loader) {
 #if ENABLE(OILPAN)
@@ -102,13 +116,20 @@ public:
             if (Heap::willObjectBeLazilySwept(m_loader))
                 return;
 #endif
-            m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
+            if (m_scriptState->contextIsValid()) {
+                v8::HandleScope handleScope(m_scriptState->isolate());

[haraken, 2015/05/28 10:57:38]

This wouldn't be needed, since the ScriptState::Scope will create the handle
scope for you.

[jochen (gone - plz use gerrit), 2015/05/28 11:16:49]

done

+                ScriptState::Scope scope(m_scriptState.get());
+                m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
+            } else {
+                m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);

[haraken, 2015/05/28 10:57:38]

Do we really want to execute this when the context associated with this task is
already disposed?

[jochen (gone - plz use gerrit), 2015/05/28 11:16:49]

yes, the loader needs to clean itself up.

[haraken, 2015/05/28 11:37:50]

In that scenario, wouldn't there be any risk of executing doUpdateFromElement in
an unexpected (or exploited) ScriptState?

Maybe we want to create a dummy ScriptState or set m_shouldBypassMainWorldCSP to
false?

+            }
         }
     }
 
     void clearLoader()
     {
         m_loader = 0;
+        m_scriptState.clear();
     }
 
     WeakPtr<Task> createWeakPtr()
@@ -120,6 +141,7 @@ private:
     ImageLoader* m_loader;
     BypassMainWorldBehavior m_shouldBypassMainWorldCSP;
     UpdateFromElementBehavior m_updateBehavior;
+    RefPtr<ScriptState> m_scriptState;
     WeakPtrFactory<Task> m_weakFactory;
 };