823228 - Move indexed properties from nsWindowSH::GetProperty to the outer window proxy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Per spec these are directly on Window, but you can't do a bareword lookup on an index, so doing them via the WindowProxy should be fine.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Move indexed property access on windows from nsWindowSH::GetProperty to the outer window proxy.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=39980b0756ba shows one orange:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug597218.js | leaked window property: 3

This is happening because Object.keys(window) actually reports the indexed properties now, an that's what testing/mochitest/browser-test.js uses to detect browser-chrome tests that leave stuff on the window.

And indeed, after I run that test, I see window.length change from 3 to 4.  That happens with or without this patch; it's just that with this patch we actually catch it.

Ian, Dão, is it possible that removeTab is not actually completely removing it somehow?   The window has a null frameElement, though...

Worst case I can work around this by changing _globalPropertyWhitelist in browser-test.js, since the problem certainly predates this patch.  But seems like it would be good to figure out what's going on here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So what I see happening here is that one child window is added via addTab(), one child window is added via TabView__initFrame() (called from an anonymous function, called from showTab(), called from pinTab()), and one child window is removed from removeTab() (or rather _endRemoveTab()).

Net result: one extra window.

Looks like the TabView thins is the tabview.html iframe being added to some deck.  Is this just some sort of lazy init that pinTab() triggers?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 699560 [details] [diff] [review]
Move indexed property access on windows from nsWindowSH::GetProperty to the outer window proxy.

Review of attachment 699560 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +569,5 @@
> +protected:
> +  nsGlobalWindow* GetWindow(JSObject *proxy)
> +  {
> +    return nsGlobalWindow::FromSupports(
> +      static_cast<nsISupports*>(js::GetProxyExtra(proxy, 0).toPrivate()));

Is there a way to assert that proxy is indeed an nsOuterWindowProxy?

@@ +575,5 @@
> +
> +  // False return value means we threw an exception.  True return value
> +  // but false "found" means we didn't have a subframe at that index.
> +  bool GetSubframeWindow(JSContext *cx, JSObject *proxy, jsid id,
> +                         JS::Value* vp, bool &found);

Inconsistent location of *s; you know my preference :)

@@ +609,5 @@
> +bool
> +nsOuterWindowProxy::getPropertyDescriptor(JSContext* cx, JSObject* proxy,
> +                                             jsid id,
> +                                             JSPropertyDescriptor* desc,
> +                                             unsigned flags)

Indentation

@@ +679,5 @@
> +                            bool *bp)
> +{
> +  if (nsCOMPtr<nsIDOMWindow> frame = GetSubframeWindow(cx, proxy, id)) {
> +    // Reject (which means throw if and only if strict) the set.
> +    // Except we don't even know whether we're strict.

Pointer to bug 803157, please.

@@ +11536,5 @@
> +  // get here when we're mid-call to nsDocShell::Destroy. See bug 640904
> +  // comment 105.
> +  if (MOZ_UNLIKELY(!global)) {
> +    xpc::Throw(cx, NS_ERROR_FAILURE);
> +    return false;

I prefer "return xpc::Throw(cx, NS_ERROR_FAILURE);", fwiw

::: dom/base/test/test_window_indexing.html
@@ +25,5 @@
> +  ok("0" in window, "We have a subframe");
> +  ok("1" in window, "We have two subframes");
> +  ok(!("2" in window), "But we don't have three subframes");
> +  window[2] = "myString";
> +  ok("2" in window, "Should be able to set expando");

Should we, really?

> Let creating be true if index is not a supported property index, and false otherwise.
> If creating is true and O does not implement an interface with an indexed property creator, then Reject.

@@ +27,5 @@
> +  ok(!("2" in window), "But we don't have three subframes");
> +  window[2] = "myString";
> +  ok("2" in window, "Should be able to set expando");
> +  Object.getPrototypeOf(window)[3] = "Hey there";
> +  ok("3" in window, "Should be walk up the proto chain");

Should be better grammar :)

@@ +92,5 @@
> +  isnot(names.indexOf("2"), -1, "And then 2, defined earlier, should be in there");
> +  is(names.indexOf("3"), -1, "But no 3; that's on the proto");
> +
> +  names = [];
> +  Math.sin(0, 0, 0);

Drop this before you land?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is there a way to assert that proxy is indeed an nsOuterWindowProxy?

If I have to, but it would involve rearranging some of the code, because I'd need to put it after the outer chrome window stuff has been declared.

> Inconsistent location of *s; you know my preference :)

Will move the Value one to the right.  ;)

> Should we, really?

Per spec, yes.

I'll fix the rest.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Should we, really?

Oh, I see what you mean.

We can file a followup to investigate changing this from the current behavior.  I worry about the compat impact of not allowing that sort of thing on the window....

Comment 7

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Looks like the TabView thins is the tabview.html iframe being added to some
> deck.  Is this just some sort of lazy init that pinTab() triggers?

Yes.

I don't think we should use _globalPropertyWhitelist here, but make browser-test.js ignore all indices returned by Object.keys.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  Ok, I can do that, I think.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I filed bug 828787 on the indexed expandos.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make browser-test.js ignore indexed properties on the window when determining whether a test leaked properties onto the global.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 699560 [details] [diff] [review]
Move indexed property access on windows from nsWindowSH::GetProperty to the outer window proxy.

Review of attachment 699560 [details] [diff] [review]:
-----------------------------------------------------------------

Nice tests :-)

::: dom/base/nsGlobalWindow.cpp
@@ +650,5 @@
> +nsOuterWindowProxy::defineProperty(JSContext* cx, JSObject* proxy,
> +                                   jsid id, JSPropertyDescriptor* desc)
> +{
> +  if (nsCOMPtr<nsIDOMWindow> frame = GetSubframeWindow(cx, proxy, id)) {
> +    // Don't define anything; we're done here

Explain a bit more why we silently fail here?

@@ +732,2 @@
>  {
>    if (id == nsDOMClassInfo::sWrappedJSObject_id &&

Didn't we determine that we didn't need this? Or am I misremembering?

@@ +776,5 @@
> +  if (!js::Wrapper::keys(cx, proxy, innerProps)) {
> +    return false;
> +  }
> +  return js::AppendUnique(cx, props, innerProps);
> +}

It's crummy that we have to do this three times for keys, getOwnPropertyNames, and enumerate. We could just use the BaseProxyHandler version for enumerate and keys. Are you concerned about transparency, or performance?

An alternative is to use some template tricks to avoid duplicating this. But It might be more complication than it saves.

@@ +795,5 @@
> +{
> +  // If we're accessing a numeric property we'll treat that as if
> +  // window.frames[n] is accessed (since window.frames === window),
> +  // if window.frames[n] is a child frame, wrap the frame and return
> +  // it without doing a security check.

I think this comment could use some updating (I'm not sure the stuff about a security check is relevant anymore), and I'm not totally sure its placement makes sense here.

@@ +805,5 @@
> +
> +  found = true;
> +  // A numeric property accessed and the numeric property is a
> +  // child frame, wrap the child frame without doing a security
> +  // check and return.

Same here.

@@ +810,5 @@
> +  return static_cast<nsGlobalWindow*>(frame.get())->WrapSelf(cx, vp);
> +}
> +
> +already_AddRefed<nsIDOMWindow>
> +nsOuterWindowProxy::GetSubframeWindow(JSContext *cx, JSObject *proxy, jsid id)

I'm concerned about this signature, especially given that the name is the same as another method which returns a boolean. Unless the compiler somehow catches coercing an already_AddRefed to a bool, calling it as a test like |if GetSubframeWindow(...)| would inadvertently cause a leak. Can we make that clearer somehow by changing the name?

@@ +818,5 @@
> +    return nullptr;
> +  }
> +
> +  nsGlobalWindow* win = GetWindow(proxy);
> +  bool unused;

Out of curiosity, what's the point aFound in this API?

@@ +831,5 @@
> +  MOZ_ASSERT(int32_t(length) >= 0);
> +  for (int32_t i = 0; i < int32_t(length); ++i) {
> +    if (!props.append(INT_TO_JSID(i))) {
> +      return false;
> +    }

Please just do props.reserve(props.length() + length) and then treat the appends as infallible (see js/public/Vector.h).

@@ +11550,5 @@
> +    xpc::Throw(cx, rv);
> +    return false;
> +  }
> +
> +  return JS_WrapValue(cx, vp);

This function doesn't make any sense to me (I'm aware it's just cribbed from nsDOMClassInfo, but I think it needs to be sorted out).

In particular, since we're requiring that |this| is an outer, |global| here is no global at all, but is actually an outer window proxy associated with the outer nsGlobalWindow. And then for some reason we pass that as the scope and pass the outer nsGlobalWindow to WrapNative, none of which makes any sense.

What is this function trying to do? If it wants the outer window wrapped into the cx compartment, then it should just wrap mJSObject and return (in which case it probably shouldn't exist at all). If it wants an inner window, it can just return that. In no case does it make sense to call WrapNative on either an inner or outer nsGlobalWindow AFAIK.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +663,1 @@
>  }

I think this semantic overloading is a bit questionable.

What doing the following on top of your changes here:

(1) Rename Is<T> to As<T>
(2) Add an Is<T> that just returns !!As<T>
(3) Use As<T> for your new use case, leaving the other callers.

Open to other ideas here. But the idea of Is(...) return anything but a boolean is a bit counter-intuitive to me.

@@ +939,5 @@
>  XPCWrappedNativeXrayTraits::resolveOwnProperty(JSContext *cx, js::Wrapper &jsWrapper,
>                                                 JSObject *wrapper, JSObject *holder, jsid id,
>                                                 PropertyDescriptor *desc, unsigned flags)
>  {
> +    // Check for indexed access on a window before we check for expandos.

Hm, so this all ties into the question of whether we want content to be able to mess with chrome expandos via cleverly-defined window hierarchies. (I remember reading a conversation between you and peter about this...somewhere). It's not like indexed expandos are particularly common, but I'm uncomfortable allowing callers to use indexed expandos and then allowing those expandos to be conditionally override via changes in the target content. Perhaps we should just forbid numeric expandos on window Xrays? I get the impression from Ms2ger's comment that they should be forbidden per spec anyway.

And if we do forbid them, the ordering here should be irrelevant, and this can move down.

@@ +940,5 @@
>                                                 JSObject *wrapper, JSObject *holder, jsid id,
>                                                 PropertyDescriptor *desc, unsigned flags)
>  {
> +    // Check for indexed access on a window before we check for expandos.
> +    int32_t index = GetArrayIndexFromId(cx, id);

Interesting. Does this mean that the old JSID_TO_INT logic in nsDOMClassInfo was broken, or just slow?

@@ +944,5 @@
> +    int32_t index = GetArrayIndexFromId(cx, id);
> +    if (IsArrayIndex(index)) {
> +        nsGlobalWindow* win =
> +            static_cast<nsGlobalWindow*>(Is<nsIDOMWindow>(wrapper));
> +        if (win) {

Is this safe? In particular, if the the argument turns out to be null here, won't the pointer twiddling implicit in static_cast potentially cause |win| to be slightly non-zero?

Also, note that |wrapper| here is an outer, and so this relies on Is<> implicitly stripping outer windows here on its way to the WN (see the /* stopAtOuter = false */ in XrayTraits::getTargetObject. Might be worth making a comment to that effect.

@@ +953,5 @@
> +                    WrapSelf(cx, &desc->value);
> +                if (ok) {
> +                    mozilla::dom::FillPropertyDescriptor(desc, wrapper, true);
> +                }
> +                return ok;

Can we ditch |ok| and put the call directly in an |if(!)| with an explicit |return true| and |return false|?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Explain a bit more why we silently fail here?

Per spec this should be treated like a set of a readonly property.  That means silently ignored in non-strict mode, throw in strict mode.  But we don't know which mode we're in (there are bugs on that), so I'm doing the non-destructive one for now.

> Didn't we determine that we didn't need this? Or am I misremembering?

We determined we didn't need this unqualified, I think.  But this code is hit for somewin.wrappedJSObject.

> Are you concerned about transparency, or performance?

I was mostly concerned for it not doing things incorrectly.  

There is no BaseProxyHandler::enumerate.

We could use BaseProxyHandler::keys, I think.  Want me to?

We could also use BaseProxyHandler::iterate, looks like: that calls keys() and enumerate().

> I think this comment could use some updating 
> Same here.

OK, will fix.

> Unless the compiler somehow catches coercing an already_AddRefed to a bool

Clang says:

  error: no viable conversion from 'already_AddRefed<nsIDOMWindow>' to 'bool'

I can come up with a different name, though.  How about WrapSubframeWindow for the version handing out a Value?

> Out of curiosity, what's the point aFound in this API?

It's the indexed getter signature WebIDL codegen uses.  It does examine the boolean in various cases...

> Please just do props.reserve(props.length() + length) and then treat the appends as
> infallible

Will do.

> What is this function trying to do?

Return the given window to script, whatever that takes.  As you note, I just copy/pasted it from classinfo.  I thought it was pretty bizarre too, but I assumed there was some deep inner/outer/compartment/whatever reason that classinfo was doing things the way it was.  Don't tell me it's just gunk that mrbkap missed!  ;)

Realistically, I think "outer window proxy wrapped into the cx compartment" is exactly what we want here.

> What doing the following on top of your changes here:

I can do that, sure.

> Perhaps we should just forbid numeric expandos on window Xrays?

I can live with that... where would be a good place to forbid that?

> Interesting. Does this mean that the old JSID_TO_INT logic in nsDOMClassInfo was broken

Broken, but I think the difference is only observable for indices that don't fit in an int Value, which are not all that likely for windows, I guess...  On the other hand, when we start forbidding indexed expandos the difference will be more observable.

> Is this safe? In particular, if the the argument turns out to be null here,

static_cast is null-safe.  It always converts null to null.

> Might be worth making a comment to that effect.

Will do.

> Can we ditch |ok|

Yes.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> > Explain a bit more why we silently fail here?
> 
> Per spec this should be treated like a set of a readonly property.  That
> means silently ignored in non-strict mode, throw in strict mode.  But we
> don't know which mode we're in (there are bugs on that), so I'm doing the
> non-destructive one for now.

Yeah, I figured. I mostly meant to add it as a comment.

> 
> > Didn't we determine that we didn't need this? Or am I misremembering?
> 
> We determined we didn't need this unqualified, I think.  But this code is
> hit for somewin.wrappedJSObject.

But shouldn't that be resolved via the Xray layer?

> 
> > Are you concerned about transparency, or performance?
> 
> I was mostly concerned for it not doing things incorrectly.  
> 
> There is no BaseProxyHandler::enumerate.

Oh, right.

> We could use BaseProxyHandler::keys, I think.  Want me to?

I'd prefer it. Up to you though.

> We could also use BaseProxyHandler::iterate, looks like: that calls keys()
> and enumerate().

Doesn't your patch already use BaseProxyHandler::iterate?


> > Unless the compiler somehow catches coercing an already_AddRefed to a bool
> 
> Clang says:
> 
>   error: no viable conversion from 'already_AddRefed<nsIDOMWindow>' to 'bool'
> 
> I can come up with a different name, though.  How about WrapSubframeWindow
> for the version handing out a Value?

I don't really like the "Wrap" name, because it implies that there might be some wrapping happening somewhere, which never happens for either brand of nsGlobalWindow. If implicit conversion to bool fails, I'm ok leaving it as-is.


> > What is this function trying to do?
> 
> Return the given window to script, whatever that takes.  As you note, I just
> copy/pasted it from classinfo.  I thought it was pretty bizarre too, but I
> assumed there was some deep inner/outer/compartment/whatever reason that
> classinfo was doing things the way it was.  Don't tell me it's just gunk
> that mrbkap missed!  ;)
> 
> Realistically, I think "outer window proxy wrapped into the cx compartment"
> is exactly what we want here.

Yes. Let's remove that function all together, and just have the callers do that to win->GetGlobalJSObject(). (That function is horribly misnamed at this point, but that's another battle).

> > Perhaps we should just forbid numeric expandos on window Xrays?
> 
> I can live with that... where would be a good place to forbid that?

In XrayWrapper<T,U>:defineProperty. 

> static_cast is null-safe.  It always converts null to null.

Good to know!

Comment 14

[Boris Zbarsky [:bzbarsky]]

> But shouldn't that be resolved via the Xray layer?

The point is that mrbkap was worried about people doing .wrappedJSObject even on things that were already .wrappedJSObject... certainly back when he wrote Xrays initially.  The pre-Xray stuff apparently allowed that, so....

> I'd prefer it. Up to you though.

Will do.

> Doesn't your patch already use BaseProxyHandler::iterate?

So it does!  OK, then.  ;)

> If implicit conversion to bool fails, I'm ok leaving it as-is.

Sounds good.

> Let's remove that function all together, and just have the callers do that
> to win->GetGlobalJSObject().

Excellent.

> In XrayWrapper<T,U>:defineProperty. 

OK.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to review comments

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing the review comments

Comment 17

[Boris Zbarsky [:bzbarsky]]

Try is green, by the way.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 703162 [details] [diff] [review]
Interdiff addressing the review comments

Review of attachment 703162 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the interdiff, and sorry I didn't get to it last night.

r=bholley with comments addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +653,5 @@
>                                     jsid id, JSPropertyDescriptor* desc)
>  {
>    if (nsCOMPtr<nsIDOMWindow> frame = GetSubframeWindow(cx, proxy, id)) {
> +    // Don't define anything; we're done here, since the spec requires
> +    // that we treat our indexed properties as readonly.

Is it all indexed access, or just for indices that exist? This does the latter, which seems a bit less sane to me, but I haven't looked it up in the spec.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +957,5 @@
> +    int32_t index = GetArrayIndexFromId(cx, id);
> +    if (IsArrayIndex(index)) {
> +        nsGlobalWindow* win =
> +            static_cast<nsGlobalWindow*>(As<nsIDOMWindow>(wrapper));
> +        // Note: As() unwrapes outer windows to get to the inner window.

unwraps.

@@ +1058,5 @@
>      }
> +
> +    // Check for an indexed property on a Window.  If that's happening, do
> +    // nothing but claim we defined it so it won't get added as an expando.
> +    int32_t index = GetArrayIndexFromId(cx, id);

That's akin to a silent failure, right? Is that correct per spec? In general we want the Xray behavior to be as close as possible to the regular behavior.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Is it all indexed access,

Per spec all, but I'm trying to mitigate risk, so will do that in a separate patch in bug 828787.

> unwraps.

Fixed.

> Is that correct per spec?

Per spec, the failure is silent in non-strict mode, throws in strict mode...  Can I tell the modes apart in here?

Comment 20

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/43511e31bf92
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a46302d16d20

Comment 21

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #19)
> Per spec, the failure is silent in non-strict mode, throws in strict mode...
> Can I tell the modes apart in here?

Waldo, what do you think about adding a jsfriendapi accessor for grabbing strict mode off of cx->currentScript()? It seems like we need it for various things in the DOM.

Comment 22

[Jeff Walden [:Waldo]]

In ES6 the various meta-object protocol operations only *attempt* to do what they're supposed to do.  If the attempt is allowed, any changes are made and the method returns true.  If the attempt isn't allowed, nothing changes and the method returns false.  Callers who want strict-mode semantics can use the success/failure return value to behave accordingly.

So, really, you don't need the current strictness, and we should change our APIs to work that way.  I'm working on this in bug 826587, for at least some of our meta-object operations.  What I've completed so far of it is a huge patch (but not too complicated a patch thus far), but it'll eliminate the need to pass around strictness everywhere.

(Plus note that consulting cx->currentScript()->strictMode or whatever wouldn't actually solve the problem, because there's no guarantee the thing performing the action is script.  You could have non-strict script do "something", that causes a set operation to happen, and whether the set succeeds or not should be up to "something", not to whatever random script caller there is.  Heck, you might not even have a script at all!)

So I don't think we should add that accessor, and we should just make our MOP work correctly in this regard.  :-)

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/43511e31bf92
https://hg.mozilla.org/mozilla-central/rev/a46302d16d20