850517 - Named window resolution doesn't dynamically update

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This was discovered by inspection in conjunction with mrbkap and bz. See bug 658909 comment 68. I'll put together a testcase now.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Sounds like a dup to me, but could be wrong.

Comment 2

[Boris Zbarsky [:bzbarsky]]

As far as I can tell this is basically bug 170799.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out child window lookup into a helper. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Switch named children resolution to resolve pure getters. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove nsWindowSH::GetProperty. v1

It no longer does anything useful.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v1

This is correct per-spec. From HTML5's "The iframe element":

"Whenever the name attribute is set, the nested browsing context's name
must be changed to the new value. If the attribute is removed, the
browsing context name must be set to the empty string."

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tests. v1

Comment 8

[Boris Zbarsky [:bzbarsky]]

As long as you're doing part 4, do you want to handle the UnsetAttr case too?

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v2

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tests. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 2 - Switch named children resolution to resolve pure getters. v2

This fixes some compartment handling issues.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724455 [details] [diff] [review]
Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v2

EmptyString() please.

And use MOZ_OVERRIDE as needed.

More importantly, you need to call UnsetAttr on the superclass.  Did the tests not catch that?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Comment on attachment 724455 [details] [diff] [review]
> Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v2
> More importantly, you need to call UnsetAttr on the superclass.  Did the
> tests not catch that?

Not the ones I ran. But I only smoketested.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724457 [details] [diff] [review]
Part 5 - Tests. v2

r=me I guess.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v3

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724530 [details] [diff] [review]
Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v3

r=me

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 724276 [details] [diff] [review]
Part 1 - Factor out child window lookup into a helper. v1

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

r=me with my comments addressed.

::: dom/base/nsDOMClassInfo.cpp
@@ +5356,5 @@
> +// Returns a child window with the given name, or null if none is found.
> +static nsIDOMWindow*
> +GetChildWindow(nsPIDOMWindow *aWin, jsid aName)
> +{
> +  const jschar *chars = ::JS_GetInternedStringChars(JSID_TO_STRING(aName));

Nit: nuke the :: while you're here.

@@ +5367,5 @@
> +                         false, true, nullptr, nullptr,
> +                         getter_AddRefs(child));
> +
> +  nsCOMPtr<nsIDOMWindow> child_win(do_GetInterface(child));
> +  return child_win;

Non-nit: this method should either use an out parameter or return already_AddRefed<nsIDOMWindow> to avoid extra refcounting and breaking XPCOM rules by returning a pointer that you just Release()d.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 724466 [details] [diff] [review]
Part 2 - Switch named children resolution to resolve pure getters. v2

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

::: dom/base/nsDOMClassInfo.cpp
@@ +5562,5 @@
>          // on the wrapper so that ::NewResolve() doesn't get called
>          // again for this property name.
> +        if (!JS_DefinePropertyById(cx, obj, id, JS::UndefinedValue(),
> +                                   ChildWindowGetter, JS_StrictPropertyStub,
> +                                   JSPROP_SHARED | JSPROP_ENUMERATE))

It's worth pointing out that this is changing behavior in the case where someone writes to one of these names, but as far as I can tell reading the spec, the new behavior is correct (we'll now throw).

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

We should probably leave bug 170799 open until we fix "removedIframeName in window" returning true if removedIframeName had been previously resolved on window. That'll be fixed by the webidl work, though. So we don't have to worry about it here.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #17)
> Non-nit: this method should either use an out parameter or return
> already_AddRefed<nsIDOMWindow> to avoid extra refcounting and breaking XPCOM
> rules by returning a pointer that you just Release()d.

I spent some time deliberating over whether this was actually an issue or not. I ended up guessing that it probably wasn't, since we often return raw pointers to things like docshells and windows that don't go away very easily. I'm happy to return an already_AddRefed, though. I'll do that now.

Comment 21

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4e5a3ec62cf8

Comment 22

[Bobby Holley (:bholley)]

Ok, so this went orange. The basic problem is tests like this one:

http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_window_indexing.html?force=1#39

The test has an iframe with both id="x" name="x", and then does |var x = $('x');|. When this happens, the |var x| ends up resolving the getter/setter, which cause the property to end up read-only, which causes things to fail.

More generally, the issue is that people expect to be able to shadow child window names. Anne says this is allowed per spec because the getter is defined on the global scope polluter, rather than the window itself. However, even if we were to do this, we'd still end up resolving the property in the initial [[GetPropertyDescriptor]] call, and since it would be readonly, the operation would fail.

As such, I'm a bit confused as to why Anne says this should work per spec. Blake says it may have something to do with a distinction between class getters and property getters, the former being shadowable in SpiderMonkey. But neither of us know what the deal is spec-wise. I'm hoping Anne can weigh in here.

I don't think we can put this on the outer window proxy, because it needs to work with non-qualified lookups. Blake suggests that we could define a clever setter that would undefine the property when invoked, but this seems nasty.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> we'd still end up resolving the property in the initial [[GetPropertyDescriptor]] call

No, we wouldn't, because var declarations don't resolve things up the proto chain.  They only check for own properties.

Assigning _without_ var wouldn't work per spec, as far as I can tell.

> I don't think we can put this on the outer window proxy

How about we just stop worrying about this bug, unless it's seriously blocking you?  The right fix is to put this on the global scope polluter and make that into a proxy.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Alternately, you could shift this to the gsp while leaving it with the getter/setter pair.  That's basically what bug 823227 is about.  I should post my WIP patches in there...

Comment 25

[Bobby Holley (:bholley)]

So, putting it on the GSP seems to work well in general. The only problem is the XOW case, because the GSP's NewResolve isn't visible to Xrays, whereas (strange as it may be) the Window's NewResolve is.

There are varying ways to fix this, but I think the best is to explicitly add GSP support to XrayWrapper. Unless peter already has plans to do something like that with his Xray work? I'd imagine not, since the GSP isn't any sort of idl-defined object at the moment.

Or maybe I'm just getting too far down the rabbit hole here?

Comment 26

[Boris Zbarsky [:bzbarsky]]

The right way to fix that is, imo, to explicitly add named-window support to Xrays for a window, just like we already did for indexed-window support.

Comment 27

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=7e497a10cbca

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 1.5 - Support named window access via Xray. v1

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 2 - Switch named children resolution to pure getters on the global scope polluter. v3

Comment 30

[Bobby Holley (:bholley)]

Apparently GetDocShell() can return null for windows that are still sorta alive. Repushed to try with a null check:

https://tbpl.mozilla.org/?tree=Try&rev=169c9fe95fdf

Comment 31

[Bobby Holley (:bholley)]

Green at last:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ab28b02b63b3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3b18b41d78
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27d4aafb847d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ec6701c27e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/886a6c69b60d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa16b1c70a3

Comment 32

[Boris Zbarsky [:bzbarsky]]

Though note you still probably want the patch I attached in bug 823227, which makes sure we always set up the gsp....

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm.  That patch doesn't help, because the code this bug added relies on the document being an HTMLDocument.  We should probably change it to get the window directly via the gsp's global or something...

In particular, these patches broke frame name lookups in SVG documents, say.

Comment 34

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #33)
> Hmm.  That patch doesn't help, because the code this bug added relies on the
> document being an HTMLDocument.

Well, it's more that that the patch in bug 823227 is just wrong, because it means that GetDocument will static_cast the private to the wrong type, no?

We should probably change it to get the
> window directly via the gsp's global or something...

Yeah, that seems reasonable.

> In particular, these patches broke frame name lookups in SVG documents, say.

Seems like the best way forward would be to fix up the patch in bug 823227 and get it landed, I'd think.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> because it means that GetDocument will static_cast the private to the wrong type, no?

No, the private is always an nsIHTMLDocument, if it's set.  If the document is not HTML, the private is null.

The point is, you can't rely on the private for getting child windows.

> Seems like the best way forward would be to fix up the patch in bug 823227 

How exactly do you propose that be done?  Again, the private is an HTML document, null if the private is not an HTML document.  I have no plans to change that short of rewriting the gsp altogether, which is not planned short-term.

So we need to either fix the broken code from this bug or back it out.  Imo.

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ab28b02b63b3
https://hg.mozilla.org/mozilla-central/rev/7f3b18b41d78
https://hg.mozilla.org/mozilla-central/rev/27d4aafb847d
https://hg.mozilla.org/mozilla-central/rev/e7ec6701c27e
https://hg.mozilla.org/mozilla-central/rev/886a6c69b60d
https://hg.mozilla.org/mozilla-central/rev/cfa16b1c70a3

Comment 37

[Philip Chee]

window.frames["accountCentralPane"] doesn't return anything. I'm pretty sure <iframe name="accountCentralPane"> exists.

However document.querySelector works:
document.querySelector("[name='accountCentralPane']")