Jan de Mooij [:jandem] Assignee
	Description • 6 years ago

ToWindowIfWindowProxy does:

    if (IsWindowProxy(obj))
        return &obj->global();

We should probably just unwrap it instead (load the window from the WindowProxy's private slot). Also, the WindowProxy optimizations in the JIT ICs and IonBuilder will probably have to guard on the WindowProxy's window.

	Luke Wagner [:luke]
	Comment 1 • 6 years ago

I remember being uncomfortable with this function when it landed because it doesn't do what its name says.  It's not giving "the window" of the WindowProxy in the sense of "the window proxied by the WindowProxy that changes with navigation"; it's giving the WindowProxy's global, which doesn't change, which is analogous to the global of a wrapper which we've independently decided is a mostly-meaningless concept.

Fortunately, there are only a few uses of this function, mostly in shell and non-central utility paths.  Assuming we can figure out what this [1] terrifying comment means, I think we should kill this function and replace it with something that speaks more to the current realm or the scripted caller's realm.

[1] https://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCVariant.cpp#35

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 6 years ago

> it's giving the WindowProxy's global, which doesn't change

Well, sort of.

In the current world, the global of a WindowProxy is always the same as the global of the Window it is proxying.  When that Window changes, we create a new WindowProxy (in the new global) and brain-transplant the old one into the relevant cross-compartment wrapper.  After which it stops testing true for IsWindowProxy, of course.

In the new world we're not going to want to do that, obviously.  But then the obvious question is: what _do_ we want the global of a WindowProxy to be? 

One option is that in the new world WindowProxy becomes a new kind of possibly-cross-compartment wrapper itself, instead of the current layering of the cross-compartment wrapper separate from the WindowProxy?  That means we may have multiple WindowProxies for a single Window, right?

Another alternative is that WindowProxy stays the way it is: one per Window.  Then we have a special cross-compartment wrapper that doesn't do any compartment-entering or security checks that we use for exposing a WindowProxy to other compartments.  We need such a wrapper for Location anyway, I suspect, because there we really do have a canonical Location object that stores expandos and such and just want to expose it to different compartments, right?  We _could_ do that with multiple Location objects that share a single expando object, I suppose, but that seems more complicated to me.

There is also the question of how we want to handle WindowProxy in an out-of-process-iframe world, of course, and which global it should get created in.  Similar for Location...  In that case there is no target global to create them in, so maybe the "multiple objects, one per compartment, with shared expando object" is the bullet we need to bite for that...

> Assuming we can figure out what this [1] terrifying comment means

So an XPCVariant holds two things, afaict:

1)  A JS Value.
2)  A C++ thingie extracted from that value (int, object, etc).

The ownership is via tracing the JS Value and holding a reference to the C++ thingie, in cases when the C++ thingie is refcounted.  So that part is all fine.

For the case of WindowProxy, the JS Value coming in is the WindowProxy.  The C++ thingie is the C++ part of the Window.

So what keeps alive the JS part of the Window?  Back when Blake added the innerization here, nothing.  That was the problem.  But nowadays the thing the variant holds on to will be the WindowProxy or a cross-compartment wrapper for the WindowProxy (after navigation).  Either way, it keeps the WindowProxy alive, which presumably keeps the JS part of the Window alive, right?

Also, I suspect (but am not sure) the inner Window C++ object itself also keeps the Window JS bit alive until it's sufficiently torn down, due to it always having "expandos" for the webidl props we add.

And when we do tear it down so it stops doing it, we never try to do any actual wrapping in nsGlobalWindowInner::WrapObject (just return our cached thing, which may be null), so the problems Blake's fix was trying to address in terms of assertions don't really arise.

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 6 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> There is also the question of how we want to handle WindowProxy in an
> out-of-process-iframe world, of course, and which global it should get
> created in.  Similar for Location...  In that case there is no target global
> to create them in, so maybe the "multiple objects, one per compartment, with
> shared expando object" is the bullet we need to bite for that...

Nika, any thoughts on this (and the rest of the first half of comment 2)?

	Nika Layzell [:nika] (ni? for response)
	Comment 4 • 6 years ago

Peter is working on this right now. I remember he has been wrestling with the issue of which compartment to put the cross-process window proxy in as well, and I don't exactly remember which compartment he selected.

Ni? peter for clarification.

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 6 years ago

Instead of cx->global() use the global associated with the WindowProxy, and IC
code should load the WindowProxy's target at runtime instead of baking in the
global object because we might transplant it.

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 6 years ago

Makes the following changes:

* The WindowProxy optimizations in the ICs and Ion now guard the WindowProxy's
  global is the script's global. Other WindowProxies are harder to optimize
  because of potential security checks based on document.domain.

* IsWindowProxyForScriptGlobal was added as helper function to consolidate the
  logic for this.

* Removes the WindowProxy optimization for CCWs. This becomes more complicated
  in the new world for various reasons and it seems better to focus on
  getting same-compartment realms working to address that use case.

	Pulsebot
	Comment 7 • 6 years ago

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a97b9b4c385e
Fix WindowProxy optimizations in the JIT for same-compartment realms. r=bzbarsky

	Daniel Varga [:dvarga]
	Comment 8 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a97b9b4c385e