Closed Bug 850517 Opened 11 years ago Closed 11 years ago

Named window resolution doesn't dynamically update

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files, 5 obsolete files)

(deleted), patch
mrbkap
: review+
Details | Diff | Splinter Review
(deleted), patch
mrbkap
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
mrbkap
: review+
Details | Diff | Splinter Review
(deleted), patch
mrbkap
: review+
Details | Diff | Splinter Review
This was discovered by inspection in conjunction with mrbkap and bz. See bug 658909 comment 68. I'll put together a testcase now.
Sounds like a dup to me, but could be wrong.
As far as I can tell this is basically bug 170799.
Attachment #724276 - Flags: review?(mrbkap)
Attachment #724277 - Flags: review?(mrbkap)
It no longer does anything useful.
Attachment #724278 - Flags: review?(mrbkap)
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."
Attachment #724279 - Flags: review?(mrbkap)
Attached patch Part 5 - Tests. v1 (obsolete) (deleted) — Splinter Review
Attachment #724280 - Flags: review?(mrbkap)
As long as you're doing part 4, do you want to handle the UnsetAttr case too?
Attachment #724279 - Attachment is obsolete: true
Attachment #724279 - Flags: review?(mrbkap)
Attachment #724455 - Flags: review?(bzbarsky)
Attached patch Part 5 - Tests. v2 (deleted) — Splinter Review
Attachment #724457 - Flags: review?(bzbarsky)
Attachment #724280 - Attachment is obsolete: true
Attachment #724280 - Flags: review?(mrbkap)
This fixes some compartment handling issues.
Attachment #724277 - Attachment is obsolete: true
Attachment #724277 - Flags: review?(mrbkap)
Attachment #724466 - Flags: review?(mrbkap)
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?
Attachment #724455 - Flags: review?(bzbarsky) → review-
(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 on attachment 724457 [details] [diff] [review]
Part 5 - Tests. v2

r=me I guess.
Attachment #724457 - Flags: review?(bzbarsky) → review+
Attachment #724455 - Attachment is obsolete: true
Attachment #724530 - Flags: review?(bzbarsky)
Comment on attachment 724530 [details] [diff] [review]
Part 4 - Propagate ifr.setAttribute('name', 'foo') to the docshell. v3

r=me
Attachment #724530 - Flags: review?(bzbarsky) → review+
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.
Attachment #724276 - Flags: review?(mrbkap) → review+
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).
Attachment #724466 - Flags: review?(mrbkap) → review+
Attachment #724278 - Flags: review?(mrbkap) → review+
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.
(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.
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.
Flags: needinfo?(annevk)
> 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.
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...
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?
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.
Flags: needinfo?(annevk)
Attachment #725117 - Flags: review?(mrbkap)
Attachment #725117 - Flags: review?(mrbkap) → review+
Attachment #725120 - Flags: review?(mrbkap) → review+
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
Blocks: 823227
Though note you still probably want the patch I attached in bug 823227, which makes sure we always set up the gsp....
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.
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
> 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.
Flags: needinfo?(bobbyholley+bmo)
Depends on: 851851
Depends on: 851887
window.frames["accountCentralPane"] doesn't return anything. I'm pretty sure <iframe name="accountCentralPane"> exists.

However document.querySelector works:
document.querySelector("[name='accountCentralPane']")
Depends on: 851924
Blocks: 851987
No longer blocks: 851987
Depends on: 851987
Flags: needinfo?(bobbyholley+bmo)
Depends on: 860494
Depends on: 857555
Depends on: 865084
Depends on: 888225
Blocks: 161061
Blocks: 823223
Blocks: 170799
Blocks: 919103
Blocks: 163924
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: