V8RecursionScope should not hold a raw pointer to ScriptExecutionContext
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2012 22:49:27 +0000 (22:49 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2012 22:49:27 +0000 (22:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=82222

Reviewed by Eric Seidel.

Source/WebCore:

Instead of holding onto ScriptExecutionContext, cache the boolean value we
care about (context->isDocument()). This avoids problems if the
context goes away as a result of the script we just ran.

Test: fast/frames/subframe-load-js-url-crash.html

* bindings/v8/V8RecursionScope.cpp:
(WebCore::V8RecursionScope::didLeaveScriptContext): Check the cached
bool instead of calling into the context.
* bindings/v8/V8RecursionScope.h:
(WebCore::V8RecursionScope::V8RecursionScope): Call isDocument from
the constructor and cache the result.
(WebCore::V8RecursionScope::~V8RecursionScope): No longer need to pass
anything to didLeaveScriptContext since it's now a member.
(V8RecursionScope): Make didLeaveScriptContext a member function and
remove its argument. Add member bool to hold the value of isDocument.

LayoutTests:

* fast/frames/resources/subframe-load-js-url-crash-iframe.html: Added.
* fast/frames/subframe-load-js-url-crash-expected.txt: Added.
* fast/frames/subframe-load-js-url-crash.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@112161 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/frames/resources/subframe-load-js-url-crash-iframe.html [new file with mode: 0644]
LayoutTests/fast/frames/subframe-load-js-url-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/subframe-load-js-url-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/V8RecursionScope.cpp
Source/WebCore/bindings/v8/V8RecursionScope.h

index ea48fead092e2c0f0274262ac70b79c4e33fcf43..63e2d8abe1d7d044b7f1dc11d695c8aed060f0db 100644 (file)
@@ -1,3 +1,14 @@
+2012-03-26  Adam Klein  <adamk@chromium.org>
+
+        V8RecursionScope should not hold a raw pointer to ScriptExecutionContext
+        https://bugs.webkit.org/show_bug.cgi?id=82222
+
+        Reviewed by Eric Seidel.
+
+        * fast/frames/resources/subframe-load-js-url-crash-iframe.html: Added.
+        * fast/frames/subframe-load-js-url-crash-expected.txt: Added.
+        * fast/frames/subframe-load-js-url-crash.html: Added.
+
 2012-03-26  Raphael Kubo da Costa  <rakuco@FreeBSD.org>
 
         [EFL] Unreviewed gardening i fast/frames, fast/html and
diff --git a/LayoutTests/fast/frames/resources/subframe-load-js-url-crash-iframe.html b/LayoutTests/fast/frames/resources/subframe-load-js-url-crash-iframe.html
new file mode 100644 (file)
index 0000000..16634eb
--- /dev/null
@@ -0,0 +1,7 @@
+<script src="../../../resources/gc.js"></script>
+<script>
+setTimeout(function() {
+    frameElement.src = "javascript:''";
+    gc();
+}, 0);
+</script>
diff --git a/LayoutTests/fast/frames/subframe-load-js-url-crash-expected.txt b/LayoutTests/fast/frames/subframe-load-js-url-crash-expected.txt
new file mode 100644 (file)
index 0000000..1042c76
--- /dev/null
@@ -0,0 +1,2 @@
+Test passes if it does not crash.
+
diff --git a/LayoutTests/fast/frames/subframe-load-js-url-crash.html b/LayoutTests/fast/frames/subframe-load-js-url-crash.html
new file mode 100644 (file)
index 0000000..16b242f
--- /dev/null
@@ -0,0 +1,3 @@
+<div>Test passes if it does not crash.</div>
+<script>if (window.layoutTestController) layoutTestController.dumpAsText();</script>
+<iframe src="resources/subframe-load-js-url-crash-iframe.html"></iframe>
index 6689b77803a754cb05916d669b4b00c38d48d8f1..b68c2e25a3f25eb17f8c2433bf5e7bcc0e409197 100644 (file)
@@ -1,3 +1,27 @@
+2012-03-26  Adam Klein  <adamk@chromium.org>
+
+        V8RecursionScope should not hold a raw pointer to ScriptExecutionContext
+        https://bugs.webkit.org/show_bug.cgi?id=82222
+
+        Reviewed by Eric Seidel.
+
+        Instead of holding onto ScriptExecutionContext, cache the boolean value we
+        care about (context->isDocument()). This avoids problems if the
+        context goes away as a result of the script we just ran.
+
+        Test: fast/frames/subframe-load-js-url-crash.html
+
+        * bindings/v8/V8RecursionScope.cpp:
+        (WebCore::V8RecursionScope::didLeaveScriptContext): Check the cached
+        bool instead of calling into the context.
+        * bindings/v8/V8RecursionScope.h:
+        (WebCore::V8RecursionScope::V8RecursionScope): Call isDocument from
+        the constructor and cache the result.
+        (WebCore::V8RecursionScope::~V8RecursionScope): No longer need to pass
+        anything to didLeaveScriptContext since it's now a member.
+        (V8RecursionScope): Make didLeaveScriptContext a member function and
+        remove its argument. Add member bool to hold the value of isDocument.
+
 2012-03-26  Dana Jansens  <danakj@chromium.org>
 
         [chromium] assertion being hit in CCLayerTreeHost::updateCompositorResources
index cad7498c676b79cd3f670bb67a3be72b91c8cc51..92be720b14c69f05f6eef6a96c2682c4b3cfcf65 100644 (file)
 #include "V8RecursionScope.h"
 
 #include "IDBPendingTransactionMonitor.h"
-#include "ScriptExecutionContext.h"
 #include "WebKitMutationObserver.h"
 
 namespace WebCore {
 
-void V8RecursionScope::didLeaveScriptContext(ScriptExecutionContext* context)
+void V8RecursionScope::didLeaveScriptContext()
 {
     // FIXME: Instrument any work that takes place when script exits to c++ (e.g. Mutation Observers).
 
@@ -49,7 +48,7 @@ void V8RecursionScope::didLeaveScriptContext(ScriptExecutionContext* context)
 #endif
 
 #if ENABLE(MUTATION_OBSERVERS)
-    if (context && context->isDocument())
+    if (m_isDocumentContext)
         WebKitMutationObserver::deliverAllMutations();
 #endif
 }
index 6cfbbab1ece9e192ac4c703b41aa1671f8339a68..3b9ad6ec11d084e7b10fe954e5cc52f5687f2f88 100644 (file)
 #ifndef V8RecursionScope_h
 #define V8RecursionScope_h
 
+#include "ScriptExecutionContext.h"
 #include "V8Binding.h"
 
 namespace WebCore {
 
-class ScriptExecutionContext;
-
 class V8RecursionScope {
     WTF_MAKE_NONCOPYABLE(V8RecursionScope);
 public:
     explicit V8RecursionScope(ScriptExecutionContext* context)
-        : m_context(context)
+        : m_isDocumentContext(context && context->isDocument())
     {
         V8BindingPerIsolateData::current()->incrementRecursionLevel();
     }
@@ -49,15 +48,15 @@ public:
     ~V8RecursionScope()
     {
         if (!V8BindingPerIsolateData::current()->decrementRecursionLevel())
-            didLeaveScriptContext(m_context);
+            didLeaveScriptContext();
     }
 
     static int recursionLevel() { return V8BindingPerIsolateData::current()->recursionLevel(); }
 
 private:
-    static void didLeaveScriptContext(ScriptExecutionContext*);
+    void didLeaveScriptContext();
 
-    ScriptExecutionContext* m_context;
+    bool m_isDocumentContext;
 };
 
 } // namespace WebCore