Open Bug 834208 Opened 9 years ago Updated 3 months ago

Should return iframes contentWindow from (HTML)Document's named getter

Categories

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

defect

Tracking

()

People

(Reporter: Ms2ger, Unassigned)

References

()

Details

Attachments

(1 file)

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.
The element there is never an "iframe" at the moment, because we don't include those in the list of named things at all.
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
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.
Attachment #8895883 - Flags: review?(michael) → review+
(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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

clearing review 'cause of smaug's comments.
Attachment #8895883 - Flags: review+
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()
    }
Attachment #8895883 - Flags: review-
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.
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?
Flags: needinfo?(bugs)
Two mIdentifierMaps would be too slow.

Could you point to the relevant parts of the spec and how they should work differently?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Hmm, making .length O(n) doesn't feel good. I guess it should be cached, and whenever mIdentifierMaps is modified, clear the caches.
Flags: needinfo?(bugs)
Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Completely rewritten patch.
Attachment #8895883 - Flags: review?(michael)
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?
Attachment #8895883 - Flags: review?(michael) → review-
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.
Flags: needinfo?(bugs)
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 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 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 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 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 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.
Flags: needinfo?(bugs)
Attachment #8895883 - Flags: review?(bugs)
Attachment #8895883 - Flags: review?(bkelly)
Hopefully someone else will be willing to pick this up and land it.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
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.
Attachment #8895883 - Flags: review?(bugs) → review-
Attachment #8895883 - Flags: review?(bkelly)
Comment on attachment 8895883 [details]
Bug 834208 - Rewrite document/window named getters per spec;

Clearing for now as olli has already r-ed.
Attachment #8895883 - Flags: review?(michael)
Priority: -- → P3
Component: DOM → DOM: Core & HTML
Duplicate of this bug: 1568907
See Also: → 1753380
Duplicate of this bug: 1753380
You need to log in before you can comment on or make changes to this bug.