[webkit-changes] [127923] trunk

Fri, 07 Sep 2012 14:44:34 -0700

Title: [127923] trunk
Revision
127923
Author
commit-qu...@webkit.org
Date
2012-09-07 14:45:23 -0700 (Fri, 07 Sep 2012)

Log Message

Enter the creationContext before creating DOM wrappers
https://bugs.webkit.org/show_bug.cgi?id=96044

Patch by Adam Barth <aba...@chromium.org> on 2012-09-07
Reviewed by Eric Seidel.

Source/WebCore:

Now that we have the creationContext available when instantiating DOM
wrappers, we should use it! After this patch, we enter the creation
context so that the DOM wrapper is created in the right context (and
therefore has the right prototype chain).

This patch would have been tested by
fast/dom/prototype-inheritance.html, but that test was removed because
it was too much work to maintain. I'll post what the diff would have
been to the results for that test as an attachment to this bug.

Note: After this patch there are a number of cleanup patches to write,
but I'll do those separately.

* bindings/js/JSGeolocationCustom.cpp:
(WebCore::JSGeolocation::getCurrentPosition):
(WebCore::JSGeolocation::watchPosition):
  - This patch also includes a minor bug fix to the JSC implementations
    of Geolocation and Notifications. Previously, JSC was using the
    current lexical context for the callback object, which makes very
    little sense as it neither cooresponds to the Geolocation (or
    Notification) object or the functions being used for the callback.

    To be consistent with how we do callbacks elsewhere in WebKit, we
    should use the context that corresponds to the Geolocation (or
    Notification) object. This change happened automatically for V8
    when this patch fixed the Geolocation (and Notifications) wrapper
    to use the right context, but for JSC we need to change these lines
    of code explicitly.
* bindings/scripts/CodeGeneratorV8.pm:
(GenerateToV8Converters):

LayoutTests:

Update the Geolocation tests to show that we now use the correct
context for these callback. Specifically, we use the context that
cooresponds to the Geolocation object itself, not whatever random
context we happened to be in when we first touched the Geolocation
object.

* fast/dom/Geolocation/callback-to-deleted-context-expected.txt:
* fast/dom/Geolocation/disconnected-frame-expected.txt:
* fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt:
* fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html:
* fast/dom/Geolocation/script-tests/callback-to-deleted-context.js:
(onSecondIframeLoaded):
* fast/dom/Geolocation/script-tests/disconnected-frame-permission-denied.js:
(onIframeUnloaded.setTimeout):
* fast/dom/Geolocation/script-tests/disconnected-frame.js:
(onIframeUnloaded.setTimeout):
(onIframeUnloaded):

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (127922 => 127923)


--- trunk/LayoutTests/ChangeLog	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/ChangeLog	2012-09-07 21:45:23 UTC (rev 127923)
@@ -1,3 +1,28 @@
+2012-09-07  Adam Barth  <aba...@chromium.org>
+
+        Enter the creationContext before creating DOM wrappers
+        https://bugs.webkit.org/show_bug.cgi?id=96044
+
+        Reviewed by Eric Seidel.
+
+        Update the Geolocation tests to show that we now use the correct
+        context for these callback. Specifically, we use the context that
+        cooresponds to the Geolocation object itself, not whatever random
+        context we happened to be in when we first touched the Geolocation
+        object.
+
+        * fast/dom/Geolocation/callback-to-deleted-context-expected.txt:
+        * fast/dom/Geolocation/disconnected-frame-expected.txt:
+        * fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt:
+        * fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html:
+        * fast/dom/Geolocation/script-tests/callback-to-deleted-context.js:
+        (onSecondIframeLoaded):
+        * fast/dom/Geolocation/script-tests/disconnected-frame-permission-denied.js:
+        (onIframeUnloaded.setTimeout):
+        * fast/dom/Geolocation/script-tests/disconnected-frame.js:
+        (onIframeUnloaded.setTimeout):
+        (onIframeUnloaded):
+
 2012-09-07  Leo Yang  <leoy...@rim.com>
 
         Appcache fallback URL match should use the longest candidate

Modified: trunk/LayoutTests/fast/dom/Geolocation/callback-to-deleted-context-expected.txt (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/callback-to-deleted-context-expected.txt	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/callback-to-deleted-context-expected.txt	2012-09-07 21:45:23 UTC (rev 127923)
@@ -4,7 +4,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS No callbacks invoked
+PASS Success callback invoked
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-expected.txt (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-expected.txt	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-expected.txt	2012-09-07 21:45:23 UTC (rev 127923)
@@ -4,8 +4,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS error.code is 2
-PASS error.message is "Geolocation cannot be used in frameless documents"
+PASS No callbacks invoked
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt	2012-09-07 21:45:23 UTC (rev 127923)
@@ -7,9 +7,7 @@
 PASS error.code is error.PERMISSION_DENIED
 PASS error.message is "User denied Geolocation"
 
-PASS error.code is error.PERMISSION_DENIED
-PASS error.message is "User denied Geolocation"
-
+PASS No callbacks invoked
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html	2012-09-07 21:45:23 UTC (rev 127923)
@@ -8,16 +8,16 @@
               testRunner.setMockGeolocationPosition(51.478, -0.166, 100);
           }
 
+          var parent = window.parent;
+
           // Make request from remote frame, this frame will be gone by the time the Geolocation
           // object attempts to invoke the callback.
           window.parent.navigator.geolocation.getCurrentPosition(function() {
-              alert('Success callback invoked unexpectedly');
-              if (window.testRunner)
-                  testRunner.notifyDone();
+              parent.testPassed('Success callback invoked');
+              parent.finishJSTest();
           }, function() {
-              alert('Error callback invoked unexpectedly');
-              if (window.testRunner)
-                  testRunner.notifyDone();
+              parent.testFailed('Error callback invoked unexpectedly');
+              parent.finishJSTest();
           });
       }
     </script>

Modified: trunk/LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js	2012-09-07 21:45:23 UTC (rev 127923)
@@ -5,11 +5,10 @@
 }
 
 function onSecondIframeLoaded() {
-    // Wait for the callbacks to be invoked
     window.setTimeout(function() {
-        testPassed('No callbacks invoked');
+        testFailed('No callbacks invoked');
         finishJSTest();
-    }, 100);
+    }, 500);
 }
 
 var iframe = document.createElement('iframe');

Modified: trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame-permission-denied.js (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame-permission-denied.js	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame-permission-denied.js	2012-09-07 21:45:23 UTC (rev 127923)
@@ -29,12 +29,13 @@
         testFailed('Success callback invoked unexpectedly');
         finishJSTest();
     }, function(e) {
-        error = e;
-        shouldBe('error.code', 'error.PERMISSION_DENIED');
-        shouldBe('error.message', '"User denied Geolocation"');
-        debug('');
+        testFailed('Error callback invoked unexpectedly');
         finishJSTest();
     });
+    setTimeout(function() {
+        testPassed('No callbacks invoked');
+        finishJSTest();
+    }, 100);
 }
 
 var iframe = document.createElement('iframe');

Modified: trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame.js (127922 => 127923)


--- trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame.js	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/LayoutTests/fast/dom/Geolocation/script-tests/disconnected-frame.js	2012-09-07 21:45:23 UTC (rev 127923)
@@ -16,11 +16,13 @@
         testFailed('Success callback invoked unexpectedly');
         finishJSTest();
     }, function(e) {
-        error = e;
-        shouldBe('error.code', '2');
-        shouldBe('error.message', '"Geolocation cannot be used in frameless documents"');
+        testFailed('Error callback invoked unexpectedly');
         finishJSTest();
     });
+    setTimeout(function() {
+        testPassed('No callbacks invoked');
+        finishJSTest();
+    }, 100);
 }
 
 var iframe = document.createElement('iframe');

Modified: trunk/Source/WebCore/ChangeLog (127922 => 127923)


--- trunk/Source/WebCore/ChangeLog	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/Source/WebCore/ChangeLog	2012-09-07 21:45:23 UTC (rev 127923)
@@ -1,3 +1,41 @@
+2012-09-07  Adam Barth  <aba...@chromium.org>
+
+        Enter the creationContext before creating DOM wrappers
+        https://bugs.webkit.org/show_bug.cgi?id=96044
+
+        Reviewed by Eric Seidel.
+
+        Now that we have the creationContext available when instantiating DOM
+        wrappers, we should use it! After this patch, we enter the creation
+        context so that the DOM wrapper is created in the right context (and
+        therefore has the right prototype chain).
+
+        This patch would have been tested by
+        fast/dom/prototype-inheritance.html, but that test was removed because
+        it was too much work to maintain. I'll post what the diff would have
+        been to the results for that test as an attachment to this bug.
+
+        Note: After this patch there are a number of cleanup patches to write,
+        but I'll do those separately.
+
+        * bindings/js/JSGeolocationCustom.cpp:
+        (WebCore::JSGeolocation::getCurrentPosition):
+        (WebCore::JSGeolocation::watchPosition):
+          - This patch also includes a minor bug fix to the JSC implementations
+            of Geolocation and Notifications. Previously, JSC was using the
+            current lexical context for the callback object, which makes very
+            little sense as it neither cooresponds to the Geolocation (or
+            Notification) object or the functions being used for the callback.
+
+            To be consistent with how we do callbacks elsewhere in WebKit, we
+            should use the context that corresponds to the Geolocation (or
+            Notification) object. This change happened automatically for V8
+            when this patch fixed the Geolocation (and Notifications) wrapper
+            to use the right context, but for JSC we need to change these lines
+            of code explicitly.
+        * bindings/scripts/CodeGeneratorV8.pm:
+        (GenerateToV8Converters):
+
 2012-09-07  Leo Yang  <leoy...@rim.com>
 
         Appcache fallback URL match should use the longest candidate

Modified: trunk/Source/WebCore/bindings/js/JSGeolocationCustom.cpp (127922 => 127923)


--- trunk/Source/WebCore/bindings/js/JSGeolocationCustom.cpp	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/Source/WebCore/bindings/js/JSGeolocationCustom.cpp	2012-09-07 21:45:23 UTC (rev 127923)
@@ -101,12 +101,12 @@
 {
     // Arguments: PositionCallback, (optional)PositionErrorCallback, (optional)PositionOptions
 
-    RefPtr<PositionCallback> positionCallback = createFunctionOnlyCallback<JSPositionCallback>(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(0));
+    RefPtr<PositionCallback> positionCallback = createFunctionOnlyCallback<JSPositionCallback>(exec, globalObject(), exec->argument(0));
     if (exec->hadException())
         return jsUndefined();
     ASSERT(positionCallback);
 
-    RefPtr<PositionErrorCallback> positionErrorCallback = createFunctionOnlyCallback<JSPositionErrorCallback>(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(1), CallbackAllowUndefined | CallbackAllowNull);
+    RefPtr<PositionErrorCallback> positionErrorCallback = createFunctionOnlyCallback<JSPositionErrorCallback>(exec, globalObject(), exec->argument(1), CallbackAllowUndefined | CallbackAllowNull);
     if (exec->hadException())
         return jsUndefined();
 
@@ -123,12 +123,12 @@
 {
     // Arguments: PositionCallback, (optional)PositionErrorCallback, (optional)PositionOptions
 
-    RefPtr<PositionCallback> positionCallback = createFunctionOnlyCallback<JSPositionCallback>(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(0));
+    RefPtr<PositionCallback> positionCallback = createFunctionOnlyCallback<JSPositionCallback>(exec, globalObject(), exec->argument(0));
     if (exec->hadException())
         return jsUndefined();
     ASSERT(positionCallback);
 
-    RefPtr<PositionErrorCallback> positionErrorCallback = createFunctionOnlyCallback<JSPositionErrorCallback>(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(1), CallbackAllowUndefined | CallbackAllowNull);
+    RefPtr<PositionErrorCallback> positionErrorCallback = createFunctionOnlyCallback<JSPositionErrorCallback>(exec, globalObject(), exec->argument(1), CallbackAllowUndefined | CallbackAllowNull);
     if (exec->hadException())
         return jsUndefined();

Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm (127922 => 127923)


--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm	2012-09-07 21:28:57 UTC (rev 127922)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm	2012-09-07 21:45:23 UTC (rev 127923)
@@ -3394,34 +3394,26 @@
 END
     }
 
-    if (IsNodeSubType($dataNode) || IsVisibleAcrossOrigins($dataNode)) {
-        push(@implContent, <<END);
+    push(@implContent, <<END);
 
-    // Enter the node's context and create the wrapper in that context.
     v8::Handle<v8::Context> context;
-    if (frame) {
-        v8::Handle<v8::Context> underlyingHandle = frame->script()->unsafeHandleToCurrentWorldContext();
-        if (v8::Context::GetCurrent() != underlyingHandle) {
-            // For performance, we enter the context only if the currently running context
-            // is different from the context that we are about to enter.
-            context = v8::Local<v8::Context>::New(underlyingHandle);
-            if (!context.IsEmpty())
-                context->Enter();
-        }
+    if (!creationContext.IsEmpty() && creationContext != v8::Context::GetCurrent()) {
+        // For performance, we enter the context only if the currently running context
+        // is different from the context that we are about to enter.
+        context = v8::Local<v8::Context>::New(creationContext);
+        ASSERT(!context.IsEmpty());
+        context->Enter();
     }
 END
-    }
 
     push(@implContent, <<END);
     wrapper = V8DOMWrapper::instantiateV8Object(frame, &info, impl.get());
 END
-    if (IsNodeSubType($dataNode) || IsVisibleAcrossOrigins($dataNode)) {
-        push(@implContent, <<END);
-    // Exit the node's context if it was entered.
+
+    push(@implContent, <<END);
     if (!context.IsEmpty())
         context->Exit();
 END
-    }
 
     push(@implContent, <<END);
     if (UNLIKELY(wrapper.IsEmpty()))

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to