Reland "Correctly set ScriptState in the image loader microtask"

Source/core/loader/ImageLoader.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2009, 2010 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #include "config.h"
 #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"
 #include "core/dom/Microtask.h"
 #include "core/events/Event.h"
 #include "core/events/EventSender.h"
 #include "core/fetch/FetchRequest.h"
 #include "core/fetch/MemoryCache.h"
 #include "core/fetch/ResourceFetcher.h"
 #include "core/frame/LocalFrame.h"
@@ skipping 42 matching lines @@
     {
         return adoptPtr(new Task(loader, updateBehavior));
     }
 
     Task(ImageLoader* loader, UpdateFromElementBehavior updateBehavior)
         : m_loader(loader)
         , m_shouldBypassMainWorldCSP(shouldBypassMainWorldCSP(loader))
         , 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.

+        }
     }
 
-    virtual void run() override
+    ~Task() override
+    {
+    }
+
+    void run() override
     {
         if (m_loader) {
 #if ENABLE(OILPAN)
             // Oilpan: this WebThread::Task microtask may run after the
             // loader has been GCed, but not yet lazily swept & finalized
             // (when this task's loader reference will be cleared.)
             //
             // Handle this transient condition by explicitly checking here
             // before going ahead with the update operation. Unsafe to do it
             // if so, as the objects that the loader refers to may have been
             // finalized by this time.
             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()
     {
         return m_weakFactory.createWeakPtr();
     }
 
 private:
     ImageLoader* m_loader;
     BypassMainWorldBehavior m_shouldBypassMainWorldCSP;
     UpdateFromElementBehavior m_updateBehavior;
+    RefPtr<ScriptState> m_scriptState;
     WeakPtrFactory<Task> m_weakFactory;
 };
 
 ImageLoader::ImageLoader(Element* element)
     : m_element(element)
     , m_image(0)
     , m_derefElementTimer(this, &ImageLoader::timerFired)
     , m_hasPendingLoadEvent(false)
     , m_hasPendingErrorEvent(false)
     , m_imageComplete(true)
@@ skipping 489 matching lines @@
 #endif
 }
 
 #if ENABLE(OILPAN)
 ImageLoader::ImageLoaderClientRemover::~ImageLoaderClientRemover()
 {
     m_loader.willRemoveClient(m_client);
 }
 #endif
 }