834208 - Should return iframes contentWindow from (HTML)Document's named getter (i.e. `<iframe name="foo">` should cause `document.foo` to be defined)

Status: RESOLVED FIXED

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

Description

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

Quoth the spec:

> If elements has only one element, and that element is an iframe
> element, then return the WindowProxy object of the nested browsing
> context represented by that iframe element, and abort these steps.

Implemented by IE/Chrome/Opera, at least.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The element there is never an "iframe" at the moment, because we don't include those in the list of named things at all.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 834208 - Rewrite document/window named getters per spec;

This matches the spec, Chrome, WebKit, and Edge.

Review commit: https://reviewboard.mozilla.org/r/167160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167160/

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Note: my last day at Mozilla is this Sunday, so an expeditious review will help ensure the change gets landed.  :)  (I will be working on Sunday, which is a workday in Israel.)

I'm guessing the WrapObject thing is wrong, because I have no idea what it does, but it seems to work.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review172352

::: dom/html/nsHTMLDocument.cpp:2335
(Diff revision 1)
> +  if ((win && !dom::WrapObject(cx, win, nullptr, &val)) ||
> +      (!win && !dom::WrapObject(cx, supp, cache, nullptr, &val))) {

nit: I don't like this conditional, it feels like it's trying to do too much.

bool ok = win 
  ? dom::WrapObject(cx, win, nullptr, &val)
  : dom::WrapObject(cx, supp, cache, nullptr, &val);
if (!ok) {
  rv.Throw(NS_ERROR_OUT_OF_MEMORY);
  return;
}

seems like it would be more clear to me.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Aryeh Gregor (:ayg) (working until August 13, then no longer with Mozilla) from comment #3)
> I'm guessing the WrapObject thing is wrong, because I have no idea what it
> does, but it seems to work.

WrapObject takes a native object and creates a JS wrapper object which allows it to be exposed to JS code.

I think that you're using it right, but I'm going to needinfo smaug to take a look and make sure you're calling the correct overload.

Comment 6

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

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review172366

::: dom/html/nsHTMLDocument.cpp:2320
(Diff revision 1)
>  nsHTMLDocument::NamedGetter(JSContext* cx, const nsAString& aName, bool& aFound,
>                              JS::MutableHandle<JSObject*> aRetval,
>                              ErrorResult& rv)
>  {
>    nsWrapperCache* cache;
>    nsISupports* supp = ResolveName(aName, &cache);

Should ResolveName return the wrappercache for window? Or is WindowNamedPropertiesHandler::getOwnPropDescriptor supposed to not return the window?

ResolveName anyhow has access to the node already so dealing with iframe there could avoid some slow QI calls.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

clearing review 'cause of smaug's comments.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review172400

::: dom/html/nsHTMLDocument.cpp:2330
(Diff revision 1)
>    }
>  
>    JS::Rooted<JS::Value> val(cx);
> -  if (!dom::WrapObject(cx, supp, cache, nullptr, &val)) {
> +  RefPtr<nsPIDOMWindowOuter> win;
> +
> +  nsCOMPtr<Element> elem = do_QueryInterface(supp);

So a few things:

1)  It's silly that ResolveName returns this nsISupports/nsWrapperCache pair, after which all callers try to get a JSObject out of it.  We should just do the to-jsobject bit inside ResolveName.  To the extent that the two callers want different behavior for iframes (do they?), we should have an arg for that.

2)  Please use HTMLIFrameElement::FromContent.  Like so, in the case when we know we have a non-null element in ResolveName:

    if (auto iframe = HTMLIFrameElement::FromContent(e)) {
      // Get iframe->GetContentWindow()
    }

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

It looks like we need to split up ResolveName for document vs. window.  This means two separate mIdentifierMaps, right?  Or should I alter mIdentifierMap somehow so that it knows about the difference between window and document?

If you spell out exactly how I should split up ResolveName, I can take a crack at implementing this before I go.  Unfortunately there's not really time for much back-and-forth, because I'm working today until a bit before when the East Coast starts working, and when I work on Sunday other people mostly aren't going to be working.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Turns out I have a few more days to work on this, but I still don't know what we want here.  ResolveName for document and window currently use the same mIdentifierMap, but they need to behave differently per spec, so how should I split them?  Two mIdentifierMaps, or alter the existing one so it stores data for both, or something else?

Comment 11

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

Two mIdentifierMaps would be too slow.

Could you point to the relevant parts of the spec and how they should work differently?

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Window: https://html.spec.whatwg.org/#dom-window-nameditem
Document: https://html.spec.whatwg.org/#dom-document-nameditem

The difference in the nodes considered is here:

Window: https://html.spec.whatwg.org/#dom-window-nameditem-filter
Document: https://html.spec.whatwg.org/#dom-document-nameditem-filter

Window includes child browsing contexts, <a>, <area>, non-exposed <object>, <frameset>, non-exposed <object>, and every element with an id, while Document doesn't.  Document includes <iframe>, while Window doesn't (although I think child browsing contexts often winds up the same in practice).

I guess the right approach is to just maintain a union in mIdentifierMaps and filter them on demand depending on whether we're in Window or Document.  If this is slow, we could make it faster by having a flag on each item to say whether it's for Window or Document or both.  Does this sound reasonable?  It would still be slower than now to get the length, because we'd have to iterate the whole list (unless we cached it), but otherwise seems like it wouldn't be much slower.

Comment 13

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

Hmm, making .length O(n) doesn't feel good. I guess it should be cached, and whenever mIdentifierMaps is modified, clear the caches.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/1-2/

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Completely rewritten patch.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review174098

Generally looks good to me, however olli mentioned in comment 13 that we should probably cache the results and clear the caches when the map is modified. I don't see any caching of this information in the current patch, so r- until that's resolved.

::: dom/base/nsIdentifierMapEntry.h:132
(Diff revision 2)
>  
>    void AddNameElement(nsINode* aDocument, Element* aElement);
>    void RemoveNameElement(Element* aElement);
>    bool IsEmpty();
> +
> +  bool IsExposedOnDocument();

Please add a comment explaining that these methods return true if any of their elements should be exposed on the document.

::: dom/html/nsHTMLDocument.h:40
(Diff revision 2)
>  } // namespace dom
>  } // namespace mozilla
>  
> +namespace mozilla {
> +namespace dom {
> +enum EForDocumentOrWindow { eForDocument, eForWindow };

nit: Please split this enum over multiple lines.

::: dom/html/nsHTMLDocument.h:111
(Diff revision 2)
>  
>    mozilla::dom::HTMLAllCollection* All();
>  
> -  nsISupports* ResolveName(const nsAString& aName, nsWrapperCache **aCache);
> +  bool ResolveName(JSContext* aCx, const nsAString& aName,
> +                   JS::Rooted<JS::Value>& aVal,
> +                   mozilla::dom::EForDocumentOrWindow aFor,

nit: Please move this before the aVal return value. I prefer keeping parameters and outparams together.

::: dom/html/nsHTMLDocument.cpp:2287
(Diff revision 2)
> -nsHTMLDocument::ResolveName(const nsAString& aName, nsWrapperCache **aCache)
> + * Helper for ResolveName.  Some of the items on our list are exposed only for
> + * window, some only for document.
> + */
> +static bool IsExposed(Element* aElem, EForDocumentOrWindow aFor)
>  {
> -  nsIdentifierMapEntry *entry = mIdentifierMap.GetEntry(aName);
> +  if (aFor == eForDocument) {

Please add a comment here pointing to where in the spec the elements which should be exposed are defined.

::: dom/html/nsHTMLDocument.cpp:2310
(Diff revision 2)
> -  if (length > 0) {
> -    if (length == 1) {
> -      // Only one element in the list, return the element instead of returning
> -      // the list.
> -      nsIContent *node = list->Item(0);
> -      *aCache = node;
> +  // Some items on the list might be inapplicable, so we need to filter them
> +  // out.  If we have at least one item that's inapplicable and at least two
> +  // items that are, we need to make a new list to return.
> +  if (list) {
> +    Element* lastValidItem = nullptr;
> +    bool validList = true;

I find this name confusing. perhaps useOriginalList, exposedItems and lastExposedItem?

::: dom/html/nsHTMLDocument.cpp:2370
(Diff revision 2)
> +      return true;
> -  }
> +    }
>  
> -  // No named items were found, see if there's one registerd by id for aName.
> -  Element *e = entry->GetIdElement();
> +    if (validItems > 1) {
> +      // The list is valid and contains more than one element, return it
> +      // XXX Sometimes we return the same list on subsequent calls, sometimes

Perhaps we want to clone it here to be consistent?

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Does this need caching, and if so, what should be cached?  I was wrong, there's no .length for named getters (that I know of), so that's not an issue.  Note that the Window named getter code already does a lot of processing to generate the relevant lists, and I'm pretty sure this code was always O(N) in the list length.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review174098

> Perhaps we want to clone it here to be consistent?

I need to check what the spec and other UAs do here.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review174098

> I need to check what the spec and other UAs do here.

I tested in other UAs, and it seems everyone returns the same object each time consistently.  So that means we need to keep around two HTMLCollection objects, unless we want the spec changed.  Since the list is live, we can't ever get away with returning the same collection object, even if currently they happen to contain the same contents.  Brief testing in Chrome, WebKit, and Edge indicate they're all interoperable on these points.  So back to the drawing board.

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/2-3/

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review174940

::: commit-message-52285:1
(Diff revisions 2 - 3)
> -Bug 834208 - Rewrite document/window named getters per spec
> +Bug 834208 - Rewrite document/window named getters per spec; r?mystor

This is another total rewrite.  It should be the last one needed, I hope, because now I also wrote extensive tests and have verified that the implementation is pretty much correct.  I think a lot of the naming is very awkward, though, and probably should be changed to something.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review175606

::: dom/base/WindowNamedPropertiesHandler.cpp:138
(Diff revision 3)
> -    }
> -    FillPropertyDescriptor(aDesc, aProxy, 0, v);
>      return true;
>    }
>  
> -  nsWrapperCache* cache;
> +  nsBaseContentList* list = entry->GetWindowContentList();

This causes window property getting to break for non-HTML elements, since the window content list only is populated by nsGenericHTMLElement.  I have to move the ID stuff up to a class that includes all elements.

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/3-4/

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/4-5/

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/5-6/

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167160/diff/6-7/

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review176244

::: commit-message-52285:17
(Diff revisions 6 - 7)
>  to be stored separately and cannot be generated on the fly from a single
>  shared list.
>  
> -All of the failures in the new rewritten named-item.html are either due
> -to our removal of <applet> in bug 1279218 (which is not yet in the
> -specs or wpt tests), or bug 834209.  We also don't implement
> +All of the failures in the new rewritten named-item.html are due to our
> +removal of <applet> in bug 1279218 (which is not yet in the specs or wpt
> +tests).  We also don't implement object/embed exposure on document per

Rebased on bug 834209.

Comment 28

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Requesting a couple of other reviewers because I'm not working for Mozilla after around 9:00 PM today (UTC+3, so 2:00 PM EDT) and I'd like to see this patch landed if possible.

Comment 29

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Hopefully someone else will be willing to pick this up and land it.

Comment 30

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

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

https://reviewboard.mozilla.org/r/167160/#review176422

::: dom/base/Element.cpp:1079
(Diff revision 7)
>      containingShadow->AddToIdTable(this, aId);
>    } else {
>      nsIDocument* doc = GetUncomposedDoc();
>      if (doc && (!IsInAnonymousSubtree() || doc->IsXULDocument())) {
>        doc->AddToIdTable(this, aId);
> +      doc->AddToWindowTable(this, aId);

Adding new hashtable lookup to a hot code path... not good. We need something better.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Clearing for now as olli has already r-ed.

Comment 34

[Daniel Holbert [:dholbert]]

[--> clarifying bug summary slightly for clarity, to make it easier to find when running into instances of this. Also, clearing see-also relationship for bug which is also a duplicate.]

Comment 35

[Edgar Chen [:edgar]]

Attachment: Bug 834208 - Align the document name getter with the spec and other browsers;

Original patch: https://hg.mozilla.org/mozreview/gecko/rev/7d93b7e3c1499b0b81d970722537b98f5d88efdd
Original author: Aryeh Gregor <[e-mail]>

According to the spec,

• https://html.spec.whatwg.org/#dom-document-nameditem
• https://html.spec.whatwg.org/#named-access-on-the-window-object

the document and window named getters return different values. This patch creates
a separated content list for document named getter while keeping the original
content list for the window named getter. Additionally, the content list now
includes both names and ids, in addition to the id list (which is still used for
other things like getElementById()).

This means one element might be on up to multiple lists, but this implementation
is necessary. Specifically, we must return a consistent live list for document.foo
and for window.foo, so they need to be stored separately and cannot be generated
on the fly from a single shared list.

Comment 36

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&revision=6ed98c8310b8a9e63e686d10dd60deafd09476fe

Comment 37

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=c512669bb6426f6ee1681a57b761d2f3cdeb788b

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c28cc6141d04
Align the document name getter with the spec and other browsers; r=smaug,devtools-reviewers,ochameau

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/51486 for changes under testing/web-platform/tests

Comment 40

[Norisz Fay [:noriszfay]]

Backed out for causing bustage on nsHTMLDocument.cpp

Backout link

Push with failures

Failure log

Comment 41

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 42

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=a2e5c2add5acfc2f26a38f183a198454aa016546

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58c66bf76010
Align the document name getter with the spec and other browsers; r=smaug,devtools-reviewers,ochameau

Comment 44

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/58c66bf76010

Comment 45

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 46

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot