On Fri Dec 6 10:42:56 2024 +0000, Gabriel Ivăncescu wrote:
This looks fine for now, pretty neat way to fix it. But I still don't think we can escape a revamp at some point. The 2 "largest" volatile objects I tested before were the storage, and doc/window, and they function differently. On doc and window, the volatile props (stuff like accessing DOM elements via props) are looked up after the prototypes, while on storage, they are looked up first, before even the prop on the object. For example:
- localStorage.getItem("test") returns null (no volatile item).
- Object.defineProperty(localStorage, "test", ...) creates normal prop
on it.
- localStorage.setItem("test", "blah") now it is `blah` (only way to
retrieve the defined prop underneat is getOwnPropertyDescriptor)
- localStorage.removeItem("test") now reveals the original defined prop
Worse is that it also hijacks deletion, so delete acts like removeItem, but if you call delete now, it won't delete the defineProperty prop (since there's no setItem so nothing to remove). For doc and window it's the opposite and if anything in the prototype chain has the name of an element id, it will be returned instead.
Thanks. This intentionally doesn't touch `PROP_EXTERN`, as deleting those requires interaction with the host object. For all jscript knows, the host may choose to ignore the delete. Fixing accessor and function properties should cover everything prototypes currently need, so those should be in good shape for the code freeze.
It could be useful to allow external properties to opt into this delete mechanism for cases where host-owned properties should always behave like regular ones. Examples include `constructor`, `prototype`, and possibly constants like `Node.TEXT_NODE`.
For cases like document and window, we might introduce a flag in `property_info` to modify name lookup. Jscript could continue searching the prototype chain and use returned info only if the search fails.
Storage properties seem more complicated. None of the solutions that come to mind feel ideal, but this is a very niche corner case. I doubt any reasonable code depends on it, so it seems safe to ignore for now. It also doesn't seem like something Proton needs to worry about.
-
Jacek Caban (＠jacek)