Changeset 97280 in webkit

Timestamp:

Oct 12, 2011 11:45:47 AM (13 years ago)

Author:

Nate Chapin

Message:

Remove logging to determine how null v8::Contexts are happening,
and check the return value of V8DOMWindowShell::initContextIfNeeded()
before using the context it initialized.
​https://bugs.webkit.org/show_bug.cgi?id=68099

Reviewed by Adam Barth.

No new tests, the only symptom is a crash without a known repro.

• bindings/v8/ScriptController.cpp:
• bindings/v8/V8DOMWindowShell.cpp:

(WebCore::V8DOMWindowShell::initContextIfNeeded): Return true

if a context already existed.

(WebCore::V8DOMWindowShell::namedItemAdded): Remove logging.

• bindings/v8/V8Proxy.cpp:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/scripts/CodeGeneratorV8.pm (modified) (1 diff)
• bindings/v8/ScriptController.cpp (modified) (1 diff)
• bindings/v8/V8DOMWindowShell.cpp (modified) (10 diffs)
• bindings/v8/V8Proxy.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-10-12  Nate Chapin  <japhet@chromium.org>
+
+        Remove logging to determine how null v8::Contexts are happening,
+        and check the return value of V8DOMWindowShell::initContextIfNeeded()
+        before using the context it initialized.
+        https://bugs.webkit.org/show_bug.cgi?id=68099
+
+        Reviewed by Adam Barth.
+
+        No new tests, the only symptom is a crash without a known repro.
+
+        * bindings/v8/ScriptController.cpp:
+        * bindings/v8/V8DOMWindowShell.cpp:
+        (WebCore::V8DOMWindowShell::initContextIfNeeded): Return true
+            if a context already existed.
+        (WebCore::V8DOMWindowShell::namedItemAdded): Remove logging.
+        * bindings/v8/V8Proxy.cpp:
+
 2011-10-06  Robert Hogan  <robert@webkit.org>
 

trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm

@@ -2727 +2727 @@
         proxy = V8Proxy::retrieve(impl->document()->frame());
         if (proxy && static_cast<Node*>(impl->document()) == static_cast<Node*>(impl)) {
-            if (proxy->windowShell()->initContextIfNeeded()) {
+            if (proxy->windowShell()->context().IsEmpty() && proxy->windowShell()->initContextIfNeeded()) {
                 // initContextIfNeeded may have created a wrapper for the object, retry from the start.
                 return ${className}::wrap(impl);

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

@@ -270 +270 @@
 void ScriptController::disableEval()
 {
-    m_proxy->windowShell()->initContextIfNeeded();
+    if (!m_proxy->windowShell()->initContextIfNeeded())
+        return;
 
     v8::HandleScope handleScope;

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

@@ -78 +78 @@
 
 namespace WebCore {
-
-// FIXME: Temporary diagnostics as to why V8 sometimes crashes with a null context in namedItemAdded().
-// See https://bugs.webkit.org/show_bug.cgi?id=68099.
-static int s_contextFailureReason = -1;
 
 static void handleFatalErrorInV8()
@@ -282 +278 @@
     // Bail out if the context has already been initialized.
     if (!m_context.IsEmpty())
-        return false;
+        return true;
 
     // Create a handle scope for all local handles.
@@ -323 +319 @@
         if (m_global.IsEmpty()) {
             disposeContextHandles();
-            s_contextFailureReason = 3;
             return false;
         }
@@ -359 +354 @@
 
     // The activeDocumentLoader pointer could be 0 during frame shutdown.
-    if (!m_frame->loader()->activeDocumentLoader()) {
-        s_contextFailureReason = 0;
+    if (!m_frame->loader()->activeDocumentLoader())
         return result;
-    }
 
     // Create a new environment using an empty template for the shadow
     // object. Reuse the global object if one has been created earlier.
     v8::Persistent<v8::ObjectTemplate> globalTemplate = V8DOMWindow::GetShadowObjectTemplate();
-    if (globalTemplate.IsEmpty()) {
-        s_contextFailureReason = 1;
+    if (globalTemplate.IsEmpty())
         return result;
-    }
 
     // Used to avoid sleep calls in unload handlers.
@@ -397 +388 @@
     result = v8::Context::New(&extensionConfiguration, globalTemplate, global);
 
-    if (result.IsEmpty())
-        s_contextFailureReason = 2;
     return result;
 }
@@ -418 +407 @@
     v8::Local<v8::Object> jsWindow = SafeAllocation::newInstance(windowConstructor);
     // Bail out if allocation failed.
-    if (jsWindow.IsEmpty()) {
-        s_contextFailureReason = 7;
+    if (jsWindow.IsEmpty())
         return false;
-    }
 
     // Wrap the window.
@@ -552 +539 @@
     // context for the new document to make property access on the
     // global object wrapper succeed.
-    initContextIfNeeded();
-
-    // Bail out if context initialization failed.
-    if (m_context.IsEmpty())
+    if (!initContextIfNeeded())
         return;
 
@@ -581 +565 @@
 void V8DOMWindowShell::namedItemAdded(HTMLDocument* doc, const AtomicString& name)
 {
-    initContextIfNeeded();
-
-    if (!isContextInitialized()) {
-#if PLATFORM(CHROMIUM)
-        // FIXME: Temporary diagnostics as to why V8 sometimes crashes with a null context below.
-        // See https://bugs.webkit.org/show_bug.cgi?id=68099.
-        PlatformSupport::histogramEnumeration("V8Bindings.nullContextState", 0, 3);
-        if (m_frame->settings() && !m_frame->settings()->isJavaScriptEnabled())
-            PlatformSupport::histogramEnumeration("V8Bindings.nullContextState", 1, 3);
-
-        if (!m_frame->script()->canExecuteScripts(NotAboutToExecuteScript))
-            PlatformSupport::histogramEnumeration("V8Bindings.nullContextState", 2, 3);
-
-        if (s_contextFailureReason >= 0 && s_contextFailureReason <= 7)
-            PlatformSupport::histogramEnumeration("V8Bindings.nullContextReason", s_contextFailureReason, 8);
-        s_contextFailureReason = -1;
-#endif
-        return;
-    }
+    if (!initContextIfNeeded())
+        return;
 
     v8::HandleScope handleScope;
@@ -630 +597 @@
     v8::Handle<v8::String> hiddenObjectPrototypeString = V8HiddenPropertyName::objectPrototype();
     // Bail out if allocation failed.
-    if (objectString.IsEmpty() || prototypeString.IsEmpty() || hiddenObjectPrototypeString.IsEmpty()) {
-        s_contextFailureReason = 4;
+    if (objectString.IsEmpty() || prototypeString.IsEmpty() || hiddenObjectPrototypeString.IsEmpty())
         return false;
-    }
 
     v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(context->Global()->Get(objectString));
     // Bail out if fetching failed.
-    if (object.IsEmpty()) {
-        s_contextFailureReason = 5;
+    if (object.IsEmpty())
         return false;
-    }
     v8::Handle<v8::Value> objectPrototype = object->Get(prototypeString);
     // Bail out if fetching failed.
-    if (objectPrototype.IsEmpty()) {
-        s_contextFailureReason = 6;
+    if (objectPrototype.IsEmpty())
         return false;
-    }
 
     context->Global()->SetHiddenValue(hiddenObjectPrototypeString, objectPrototype);
@@ -656 +617 @@
 {
     // Not in cache.
-    initContextIfNeeded();
+    if (!initContextIfNeeded())
+        return notHandledByInterceptor();
+
     v8::Context::Scope scope(m_context);
     v8::Local<v8::Function> function = V8DOMWrapper::getConstructor(type, getHiddenObjectPrototype(m_context));

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

@@ -246 +246 @@
 {
     // FIXME: This will need to get reorganized once we have a windowShell for the isolated world.
-    windowShell()->initContextIfNeeded();
+    if (!windowShell()->initContextIfNeeded())
+        return;
 
     v8::HandleScope handleScope;