Changeset 142565 in webkit

Timestamp:

Feb 11, 2013 6:06:13 PM (11 years ago)

Author:

haraken@chromium.org

Message:

[V8] ScheduledAction::m_context can be empty, so we shouldn't
retrieve an Isolate by using m_context->GetIsolate()
​https://bugs.webkit.org/show_bug.cgi?id=109523

Reviewed by Adam Barth.

Chromium bug: ​https://code.google.com/p/chromium/issues/detail?id=175307#makechanges

Currently ScheduledAction is retrieving an Isolate by using m_context->GetIsolate().
This can crash because ScheduledAction::m_context can be empty. Specifically,
ScheduledAction::m_context is set to ScriptController::currentWorldContext(),
which can return an empty handle when a frame does not exist. In addition,
'if(context.IsEmpty())' in ScheduledAction.cpp implies that it can be empty.

Alternately, we should pass an Isolate explicitly when a ScheduledAction is instantiated.

No tests. The Chromium crash report doesn't provide enough information
to reproduce the bug.

• bindings/v8/ScheduledAction.cpp:

(WebCore::ScheduledAction::ScheduledAction):
(WebCore):
(WebCore::ScheduledAction::~ScheduledAction):

• bindings/v8/ScheduledAction.h:

(ScheduledAction):

• bindings/v8/custom/V8DOMWindowCustom.cpp:

(WebCore::WindowSetTimeoutImpl):

• bindings/v8/custom/V8WorkerContextCustom.cpp:

(WebCore::SetTimeoutOrInterval):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/v8/ScheduledAction.cpp (modified) (2 diffs)
• bindings/v8/ScheduledAction.h (modified) (2 diffs)
• bindings/v8/custom/V8DOMWindowCustom.cpp (modified) (2 diffs)
• bindings/v8/custom/V8WorkerContextCustom.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-11  Kentaro Hara  <haraken@chromium.org>
+
+        [V8] ScheduledAction::m_context can be empty, so we shouldn't
+        retrieve an Isolate by using m_context->GetIsolate()
+        https://bugs.webkit.org/show_bug.cgi?id=109523
+
+        Reviewed by Adam Barth.
+
+        Chromium bug: https://code.google.com/p/chromium/issues/detail?id=175307#makechanges
+
+        Currently ScheduledAction is retrieving an Isolate by using m_context->GetIsolate().
+        This can crash because ScheduledAction::m_context can be empty. Specifically,
+        ScheduledAction::m_context is set to ScriptController::currentWorldContext(),
+        which can return an empty handle when a frame does not exist. In addition,
+        'if(context.IsEmpty())' in ScheduledAction.cpp implies that it can be empty.
+
+        Alternately, we should pass an Isolate explicitly when a ScheduledAction is instantiated.
+
+        No tests. The Chromium crash report doesn't provide enough information
+        to reproduce the bug.
+
+        * bindings/v8/ScheduledAction.cpp:
+        (WebCore::ScheduledAction::ScheduledAction):
+        (WebCore):
+        (WebCore::ScheduledAction::~ScheduledAction):
+        * bindings/v8/ScheduledAction.h:
+        (ScheduledAction):
+        * bindings/v8/custom/V8DOMWindowCustom.cpp:
+        (WebCore::WindowSetTimeoutImpl):
+        * bindings/v8/custom/V8WorkerContextCustom.cpp:
+        (WebCore::SetTimeoutOrInterval):
+
 2013-02-11  Adenilson Cavalcanti  <cavalcantii@gmail.com>
 

trunk/Source/WebCore/bindings/v8/ScheduledAction.cpp

@@ -50 +50 @@
 namespace WebCore {
 
-ScheduledAction::ScheduledAction(v8::Handle<v8::Context> context, v8::Handle<v8::Function> function, int argc, v8::Handle<v8::Value> argv[])
+ScheduledAction::ScheduledAction(v8::Handle<v8::Context> context, v8::Handle<v8::Function> function, int argc, v8::Handle<v8::Value> argv[], v8::Isolate* isolate)
     : m_context(context)
     , m_function(function)
     , m_code(String(), KURL(), TextPosition::belowRangePosition())
+    , m_isolate(isolate)
 {
-    v8::Isolate* isolate = m_context->GetIsolate();
     m_args.reserveCapacity(argc);
     for (int i = 0; i < argc; ++i)
-        m_args.append(v8::Persistent<v8::Value>::New(isolate, argv[i]));
+        m_args.append(v8::Persistent<v8::Value>::New(m_isolate, argv[i]));
+}
+
+ScheduledAction::ScheduledAction(v8::Handle<v8::Context> context, const String& code, const KURL& url, v8::Isolate* isolate)
+    : m_context(context)
+    , m_code(code, url)
+    , m_isolate(isolate)
+{
 }
 
@@ -64 +71 @@
 {
     for (size_t i = 0; i < m_args.size(); ++i) {
-        m_args[i].Dispose(m_context->GetIsolate());
+        m_args[i].Dispose(m_isolate);
         m_args[i].Clear();
     }

trunk/Source/WebCore/bindings/v8/ScheduledAction.h

@@ -46 +46 @@
 class ScheduledAction {
 public:
-    ScheduledAction(v8::Handle<v8::Context>, v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]);
+    ScheduledAction(v8::Handle<v8::Context>, v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[], v8::Isolate*);
+    ScheduledAction(v8::Handle<v8::Context>, const String&, const KURL&, v8::Isolate*);
+    ~ScheduledAction();
 
-    ScheduledAction(v8::Handle<v8::Context> context, const String& code, const KURL& url = KURL())
-        : m_context(context)
-        , m_code(code, url)
-    {
-    }
-
-    ~ScheduledAction();
     void execute(ScriptExecutionContext*);
 
@@ -67 +62 @@
     Vector<v8::Persistent<v8::Value> > m_args;
     ScriptSourceCode m_code;
+    v8::Isolate* m_isolate;
 };
 

trunk/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp

@@ -122 +122 @@
         // params is passed to action, and released in action's destructor
         ASSERT(imp->frame());
-        OwnPtr<ScheduledAction> action = adoptPtr(new ScheduledAction(imp->frame()->script()->currentWorldContext(), v8::Handle<v8::Function>::Cast(function), paramCount, params));
+        OwnPtr<ScheduledAction> action = adoptPtr(new ScheduledAction(imp->frame()->script()->currentWorldContext(), v8::Handle<v8::Function>::Cast(function), paramCount, params, args.GetIsolate()));
 
         // FIXME: We should use OwnArrayPtr for params.
@@ -132 +132 @@
             return v8Integer(0, args.GetIsolate());
         ASSERT(imp->frame());
-        id = DOMTimer::install(scriptContext, adoptPtr(new ScheduledAction(imp->frame()->script()->currentWorldContext(), functionString)), timeout, singleShot);
+        id = DOMTimer::install(scriptContext, adoptPtr(new ScheduledAction(imp->frame()->script()->currentWorldContext(), functionString, KURL(), args.GetIsolate())), timeout, singleShot);
     }
 

trunk/Source/WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp

@@ -71 +71 @@
         }
         WTF::String stringFunction = toWebCoreString(function);
-        timerId = DOMTimer::install(workerContext, adoptPtr(new ScheduledAction(v8Context, stringFunction, workerContext->url())), timeout, singleShot);
+        timerId = DOMTimer::install(workerContext, adoptPtr(new ScheduledAction(v8Context, stringFunction, workerContext->url(), args.GetIsolate())), timeout, singleShot);
     } else if (function->IsFunction()) {
         size_t paramCount = argumentCount >= 2 ? argumentCount - 2 : 0;
@@ -81 +81 @@
         }
         // ScheduledAction takes ownership of actual params and releases them in its destructor.
-        OwnPtr<ScheduledAction> action = adoptPtr(new ScheduledAction(v8Context, v8::Handle<v8::Function>::Cast(function), paramCount, params));
+        OwnPtr<ScheduledAction> action = adoptPtr(new ScheduledAction(v8Context, v8::Handle<v8::Function>::Cast(function), paramCount, params, args.GetIsolate()));
         // FIXME: We should use a OwnArrayPtr for params.
         delete [] params;