Use the current context as the creation context for cross-origin objects.

third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp

Index: third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp b/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
index 2134492acdd163242964c52f0d597abadc66f88b..c915cd2ad97e7be37dc71ce73245fa174da77abd 100644
--- a/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
@@ -50,6 +50,7 @@
 #include "core/frame/ImageBitmap.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
+#include "core/frame/Location.h"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
@@ -64,6 +65,53 @@
 
 namespace blink {
 
+void V8Window::locationAttributeGetterCustom(
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  v8::Isolate* isolate = info.GetIsolate();
+  v8::Local<v8::Object> holder = info.Holder();
+
+  DOMWindow* window = V8Window::toImpl(holder);
+  Location* location = window->location();
+  DCHECK(location);
+
+  // Keep the wrapper object for the return value alive as long as |this|
+  // object is alive in order to save creation time of the wrapper object.
+  if (DOMDataStore::setReturnValue(info.GetReturnValue(), location))

[dcheng, 2017/01/24 23:12:11]

Now that a Location object only exposes a consistent set of wrappers (either
NewRemoteInstance() wrappers, for a Location for a RemoteDOMWindow) or a
standard DOM wrapper, we can cache the wrappers using DOMDataStore.

[haraken, 2017/01/25 03:24:12]

Can we add some dcheck to check that we're returning a wrapper of an intended
window?

[dcheng, 2017/01/26 02:12:03]

Can you elaborate what you mean here? Do you mean I should DCHECK that the
location pointer stored in the internal field matches the location object here?

[haraken, 2017/01/26 05:59:57]

Ah, sorry. I realized my comment doesn't make sense -- please ignore :)

+    return;
+
+  v8::Local<v8::Value> wrapper;
+
+  // Note that this check is gated on whether or not |window| is remote, not
+  // whether or not |window| is cross-origin. If |window| is local, the
+  // |location| property must always return the same wrapper, even if the
+  // cross-origin status changes by changing properties like |document.domain|.
+  if (window->isRemoteDOMWindow()) {
+    DOMWrapperWorld& world = DOMWrapperWorld::current(isolate);
+    const auto* wrapperTypeInfo = location->wrapperTypeInfo();
+    v8::Local<v8::Object> newWrapper =
+        wrapperTypeInfo->domTemplate(isolate, world)
+            ->NewRemoteInstance()
+            .ToLocalChecked();
+
+    DCHECK(!DOMDataStore::containsWrapper(location, isolate));
+    wrapper = V8DOMWrapper::associateObjectWithWrapper(
+        isolate, location, wrapperTypeInfo, newWrapper);
+  } else {
+    wrapper = ToV8(location, holder, isolate);
+
+    // Keep the wrapper object for the return value alive as long as |this|
+    // object is alive in order to save creation time of the wrapper object.

[dcheng, 2017/01/24 23:12:11]

I'm not 100% sure about this, but we could try caching this for all windows. Any
opinions here?

[haraken, 2017/01/25 03:24:12]

Actually this caching is already wrong in many ways:

- The caching is not a performance optimization to save the creation time. The
caching is needed for correctness to keep window.location alive while the window
object is alive.

- So we have to generate the caching code for all DOM attributes. However, we
generate it only for read-only attributes. That's a bug.

- So you should enable the caching for all windows.

A middle-term fix is to replace the caching with the wrapper tracing.

[dcheng, 2017/01/26 02:12:03]

I'm trying to understand why this is important. If there is an object keeping
the Location wrapper live, then we will use the already-created wrapper. If
there is no reference keeping the Location wrapper live, the fact that the
wrapper changes is unobservable. Am I missing something?
This isn't specific to window.location then, right? Perhaps a separate CL would
be more appropriate? I'm especially nervous here, because setting the hidden
value depends on the current context, which means we're setting hidden values on
a window object from a cross-origin context.

[haraken, 2017/01/26 05:59:57]

Imagine that a user sets window.location.foo="aaa".

If we don't have a hidden reference from window to window.location, the wrapper
may be collected in the next GC. Then the wrapper will be recreated when
window.location is accessed next. Then window.location.foo is lost (returns
undefined).
Yeah, the hidden reference is already broken in many ways, so I think it's fine
to address it in a follow-up.

[dcheng, 2017/01/26 06:30:19]

Ah-ha, I understand now.
So the interesting part here is Location cannot be reused, so a Location is tied
to either a RemoteDOMWindow or a LocalDOMWindow.

However, if the location is for a RemoteDOMWindow, we never need to cache the
wrapper, because it is not possible to set any JS properties on it. So the
effects are never observable.

Given that, I've added a comment to explain why it's OK to skip this for a
Location object for a RemoteFrame, with a TODO to address this and the hidden
value work in a followup.

[haraken, 2017/01/26 09:27:09]

It might still be observable when the remote Location wrapper is stored in a
WeakMap. If the wrapper is lost, it will be removed from the WeakMap.

Either way, this is already widely broken in the code base, so you don't need to
worry about it at the moment.

[Yuki, 2017/01/26 10:41:53]

I'd argue on this point.  IIUC, the spec doesn't require to keep
|window.location.expando|.  I agree that web developers would be surprised,
though.

Unless [SameObject] is specified on a DOM attribute, it's not guaranteed that
the DOM attribute returns the same object.  So it's totally okay from a PoV of
the spec that we create a new wrapper and return it.  It's okay to return a
different object every time.

My point is "it's a nice-to-have".  The spec says it's okay.  Less surprising,
it's better, though.

+    const char kKeepAliveKey[] = "KeepAlive#Window#location";
+    V8HiddenValue::setHiddenValue(
+        ScriptState::current(isolate), holder,
+        v8AtomicString(isolate,
+                       StringView(kKeepAliveKey, sizeof kKeepAliveKey)),
+        wrapper);
+  }
+
+  v8SetReturnValue(info, wrapper);
+}
+
 void V8Window::eventAttributeGetterCustom(
     const v8::FunctionCallbackInfo<v8::Value>& info) {
   LocalDOMWindow* impl = toLocalDOMWindow(V8Window::toImpl(info.Holder()));