On Wed Nov 8 14:51:03 2023 +0000, Jacek Caban wrote:
First of all, you're misinterpreting events.c results, we're incompatible with native in a different way and doc node reference to outer window just hides it, see: https://testbot.winehq.org/JobDetails.pl?Key=139652 CC is not a magic fix for everything. DOM part of reference graph is already very large: entire DOM tree needs to be kept alive as long as at least a single DOM node reference is alive and that includes all sub-frames. If anything, it would be interesting to see if we can break this graph, not extend it carelessly. For example, Gecko has non-owning references to the document from nodes, while we use CC reference. Instead, this MR merges reference graphs from all previous documents, meaning that any reference to a node that's no longer a part of DOM tree will keep alive all DOM trees, including their sub-frames and all DOM trees orphaned by navigation. It feels like a major step backwards. Navigation is a great opportunity to break references and allow subsequent collection, I think we really should use this opportunity. And note that what I describe is not just about making leaks "leak less". There are perfectly valid situations where references like you propose would actually cause leaks. Imagine the main documents runs a code like `document.myprop = external.createMyProp(iframe.contentWindow)` and `createMyProp` returns an object that holds a reference to the argument (`iframe.contentWindow`). If child window would store a strong reference to parent window, that would be a cycle and it wouldn't be collected even if entire document is orphaned by navigation. That said, there is definitely a room for improvements for inner/outer windows. I started refactoring this area a few years ago, but never had a chance to finish it. The huge part that prevents some of changes is binding code, which is rather ugly ATM. I imagine that it would be interesting to refactor it and, as a part of that, move the moniker, URL, etc. from outer window to the inner one. We could probably also avoid accessing outer window in a number of other places by storing Gecko inner window inside our inner window. That could fix a few tests that you mention. And yes, I think we'd want to try to get rid of outer window pointer from document nodes but it's tricky. The test that you quoted is not a proof that it's not possible, but if we still need them after all, using weak references would be much better. Anyway, I think that a strong parent reference just hides problems and proper fixes can easily become a time sink. Does it block your "proxy" patches in any way?
I see, the test is kind of misleading, I can probably change it (even if we don't fix it in Wine) and add a todo_wine of course (since now it will block waiting for it expecting to load).
I don't think this necessarily blocks anything, it does simplify some things though, because we can uniformly access it from everywhere inner window is used, but if we move more of the outer window fields to the inner window, then it won't really matter.
And I wanted to get rid of most of these revamps first before code freeze at least, it will make it easier to rebase then to stuff like Proton, especially if they need total overhauls based on reviews (as you can guess from this now).
On second thought, what should I do about the document's get_parentWindow? It's not the same doc (unlike with iframe test), but it still returns the outer window.
Should I keep a strong ref from the document to the outer window in doc->outer_window and use it there? Although it won't make much difference than holding it from inner windows, though.
Alternatively, I could keep some sort of weak ref to the outer_window from the doc, but it would probably require it to be added to a list of detached docs on the outer window (so it knows to detach them when it gets killed). Is that better?