On Fri Dec 1 19:13:35 2023 +0000, Jacek Caban wrote:
This is still not reviewable in its current form. Neither I nor you can know what's the correct solution without doing the research. Saying that you will do the research only if I promise to accept one or another patch just doesn't make sense. It all depends on outcome of the research; until we have enough information we need to keep it open minded. Claiming that it's not needed because you're interested only in a few random tests also doesn't make sense when you change data structures used module-wide (it's even worse when tests themselves are questionable). Doing code changes like that is easy, the hard part if figuring how to do that. It's unfair to skip the hard part, refuse to do it when asked and complain that the reviewer doesn't do it for you. It's supposed to be author's responsibility. Anyway, I did that just so we can move forwards, see !4574. What I found is:
- On IE9+ it's not even allowed to touch detached document, not even
compare it to anything.
- In old IE mode any attempt to call such document is an error.
- In old IE mode, detached document returns true when it's compared to
current document, but still fails on any attempt to call it (unlike the current document). Conclusions:
- Arguments like "I need to get it committed because it would be hard to
debug if something needs it" are pointless, web pages simply have no way of depending on what you did.
- Weird crashes observed are probably a by-product of implementation of
old IE modes in native; although those APIs are exposed to C it seems that they are just broken there, so trying to read too much out of it is pointless.
- We should probably just error-out somewhere in JS binding layer.
That's not a task for now; adding some checks to C variant is still not wrong (they are exposed with C interfaces after all), so that's what I did for now.
- Storing outer window for longer than needed is not justified.
I hope that explains you why skipping proper research is not a good idea.
I understand your points, but I'm unsure how that affects the last push? I got rid of the patch that changes outer windows or keeps their refs around for longer, the other patches that are still not in !4574 were definitely fine (and even what you suggested, btw). If not, which one do you think is "not reviewable"?
The only one that comes to my mind is the `doc->outer_window` change but, it's currently broken because it **does** keep ref to the outer window for "longer than it should" (i.e. goes against what you just said), and even worse it does not even hold any sort of ref, so it can access invalid memory.
So all my patch does is make it become NULL when the inner window is detached, isn't that what you suggested?!
The storage event patch, again, just does what you suggested: it checks if we're detached and skips sending the event altogether (I did put a FIXME comment, but that's just a comment…).
Can you please tell me which patch is so wrong now?