Bobby Holley (:bholley) Assignee
	Description • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 11 years ago

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

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

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

	Bobby Holley (:bholley) Assignee
	Comment 5 • 11 years ago

It no longer does anything useful.

	Bobby Holley (:bholley) Assignee
	Comment 6 • 11 years ago

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."

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 11 • 11 years ago

This fixes some compartment handling issues.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

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?

	Bobby Holley (:bholley) Assignee
	Comment 13 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

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

r=me I guess.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

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

r=me

	Blake Kaplan (:mrbkap) (inactive)
	Comment 17 • 11 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 18 • 11 years ago

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).

	Blake Kaplan (:mrbkap) (inactive)
	Comment 19 • 11 years ago

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.

	Bobby Holley (:bholley) Assignee
	Comment 20 • 11 years ago

(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.

	Bobby Holley (:bholley) Assignee
	Comment 21 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 22 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 11 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

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...

	Bobby Holley (:bholley) Assignee
	Comment 25 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

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.

	Bobby Holley (:bholley) Assignee
	Comment 27 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 30 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 31 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 11 years ago

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.

	Bobby Holley (:bholley) Assignee
	Comment 34 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 11 years ago

> 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.

	Phil Ringnalda (:philor)
	Comment 36 • 11 years ago

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

	Philip Chee
	Comment 37 • 11 years ago

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

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