Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

May return the same HTMLCollection object #706

Open
annevk opened this issue Oct 18, 2018 · 12 comments
Open

May return the same HTMLCollection object #706

annevk opened this issue Oct 18, 2018 · 12 comments

Comments

@annevk
Copy link
Member

annevk commented Oct 18, 2018

https://dom.spec.whatwg.org/#concept-getelementsbytagname has

When invoked with the same argument, and as long as root’s node document’s type has not changed, the same HTMLCollection object may be returned as returned by an earlier call.

and variants on that. Per @rniwa this is considered a bug by WebKit these days. It'd be nice to stop exposing GC here to the extent that's still happening and define the lifetime of these objects more properly.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2018

Context: whatwg/html#4098.

@domenic
Copy link
Member

domenic commented Oct 18, 2018

This doesn't expose GC as long as the browser holds a strong reference to the returned collections, which might make sense for live collections since they are pretty lightweight (consisting of basically a tag name + JS object overhead). I'm not sure whether existing implementations use strong or weak references.

I think changing this should have a larger discussion, if that's desired. So /cc @bzbarsky @tkent-google @dstorey.

(Aside: it's funny to think that we could have had a weak reference polyfill for many years, at least in some browsers, just by storing expandos on collections returned by these methods...)

@bzbarsky
Copy link

Browsers don't hold strong references to the returned collections right now, as far as I know. Gecko certainly does not.

Live collections cache things (at least in Gecko); they don't recompute on every access. So the overhead of a live collection is:

  1. Whatever it has cached at the moment.
  2. Whatever mutation observers it needs to add to invalidate its cache as needed.
  3. The memory for the collection itself.
  4. If holding strong references, whatever data structure holds those strong references.

It's not 100% clear what the desired lifetime of a collection would be if we held strong references to them. Should it basically be tied to the lifetime of the node the collection is rooted at?

@annevk
Copy link
Member Author

annevk commented Oct 18, 2018

Yeah, I guess so, or a fresh (wrapper) object each time.

@domenic
Copy link
Member

domenic commented Oct 18, 2018

FWIW https://jsbin.com/kabidac/edit?html,console seems to work "as expected" (i.e., a weak ref polyfill) in Edge, but in Firefox and Chrome I can never get the after result to be undefined.

@bzbarsky
Copy link

If I use a 10s timeout instead of 1s I get undefined in Firefox.

For what it's worth, in Firefox you can set the "javascript.options.mem.log" preference to true to get logging in the view shown under "Tools > Web Developer > Browser Console". In this case, I see no GC activity on that page within the 1s timeout, which is why you see the results you see. I'm pretty sure we deprioritize GC activity during and right after pageload to improve responsiveness in Gecko...

@tkent-google
Copy link
Collaborator

tkent-google commented Oct 19, 2018

I'd like to change "may" to "must", or leave it "may".
Creating new HTMLCollection instances everytime will increase management cost.

@bzbarsky
Copy link

"must" would prevent the browser from ever collecting them, which also increases costs, right? This is why browsers have the behavior they have now: They avoid creating new collections if they can avoid it, but still get rid of old ones so their listeners don't keep slowing down DOM operations.

@tkent-google
Copy link
Collaborator

"must" would prevent the browser from ever collecting them, which also increases costs, right?

Right. It's worse than "may".
I think Blink has strong references from elements to HTMLCollections. So it's ok to change it to "must" to stop exposing GC behavior.

@bzbarsky
Copy link

I think Blink has strong references from elements to HTMLCollections.

Where exactly?

@tkent-google
Copy link
Collaborator

Where exactly?

blink::Node has a strong reference to blink::NodeRareData.
The code. It looks a raw pointer, however it is treated as a strong reference in Node::Trace().

blink::NodeRareData has a strong reference to blink::NodeListsNodeData.
The code

blink::NodeListsNodeData has a strong reference to TagCollectionNSCache, which is HeapHashMap<QualifiedName, TraceWrapperMember<TagCollectionNS>. TagCollectionNS inherits from HTMLCollection.
The code
Note that TraceWrapperMember<> means strong references to both of a C++ object and a JavaScript object wrapping it.

@bzbarsky
Copy link

Hmm. I thought I had gotten undefined in https://jsbin.com/kabidac/edit?html,console in Chrome, but I'm not reproducing that right now. @domenic did you end up seeing undefined in Chrome there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants