Re: [PATCH v4 0/8] MR4380: mshtml: Keep inner window's link to the outer window until it's destroyed.

[Gabriel Ivăncescu (＠insn)]

On Tue Nov 28 14:17:15 2023 +0000, Jacek Caban wrote:
> > As for the "hide problems in the future", I actually think the exact
>
> opposite. Right now, we use the document's outer_window in same way,
> except it's completely broken and can access invalid memory if the outer
> window happens to die, which would be very hard to debug (especially
> considering most apps using mshtml are bloated and slow with millions of frameworks).
> Yes, it would be nice to fix that. One fix for that is to simply reset
> the pointer to NULL. My understanding is that the actual reason we can't
> do that is iframe navigation test and we already know that the test
> works because on native, the document interface doesn't change. So what
> you do is essentially working around that problem, not fixing it.

Resetting the point to NULL is the same as using the `doc->window->base.outer_window`, which has potential for a lot more regressions (I don't mean the test). How is that better?!?

I don't know why you think this is so risky; as mentioned in the MR's message, those are the only things that would be affected, and they're all either fixed or handled (handled means same behavior as before). I spent time carefully reviewing them, I'd like that not to be a waste and just dismissed like that.

Sure I might have missed some, but it's pretty unlikely. I also tested real world apps and it seemed fine. I was hoping a review to "double check" it would provide the extra eyes on it, instead of being dismissed, even though it's correct.

Also another argument for it is that, since I suspect it will have to be fixed properly at some point, the sooner we do it, the easier it will be to review, which is now. The more complicated mshtml becomes (necessarily, to implement new features), the harder it will be to revamp this. Why not do it now?

It just seems to me that you dismiss it because of potential regressions without actually analyzing what could cause them at all, since my goal with this patch and the MR message was definitely to keep same behavior as before *except* in places where I fixed it (and mentioned in the message). *If* that's not the case everywhere I'd like to know, too, but I did carefully review it. Would you please reconsider?