[PATCH v3 0/11] MR5444: jscript: Do not allocate dispex_prop_t for indexed props.

newer
[PATCH v36 0/9] MR3103: server:...

Gabriel Ivăncescu

19 Apr 2024 19 Apr '24

7:23 p.m.

-- v3: jscript: Remove PROP_IDX dispex props by handling them in object prop methods. jscript: Move filling the PROTREF into a helper. jscript: Simplify get_flags to only check whether it's enumerable. jscript: Get rid of on_put in the object vtbl. jscript: Inline prop_put. jscript: Inline prop_get. jscript: Inline invoke_prop_func and invoke PROTREFs using their vtbl method. jscript: Inline delete_prop. jscript: Use mandatory methods in the object vtbl to operate on props found jscript: Use mandatory methods in the object vtbl to operate on props mshtml/tests: Test redefining a writable indexed prop. https://gitlab.winehq.org/wine/wine/-/merge_requests/5444

Show replies by date

Gabriel Ivăncescu

19 Apr 19 Apr

...
Well this MR technically isn't about that, is it? I think it should, see my earlier comments. And having something like this is clearly the most "extensible" since it allows overriding every prop's behavior. There are many solutions, we're looking for the one that fits our needs best. I can't review that aspect in its current form and I don't share your confidence about it. I pushed new version with a small fix for arguments.

I also rewrote the Typed Arrays and jscript proxy patches to make use of this. It's on my wine's branch `jscript-proxies`. The patches of note are cb615b32c4cd8cd9afed43bf1882df9139e6d365 and be63c5738979878a154b65e142983c9802feef4f. Note that they aren't split yet, since I keep rewriting them for now, it would make it too much wasted effort. But I don't think a full review of them is necessary, just the parts handling the props, right? Some things to note: * I got rid of PropDefineOverride and the "this" pointer for PropInvoke. * With respect to props, there are no special cases anymore for proxies, it's all handled in the props methods in the vtbl (from this MR). * Only one new method had to be added, `prop_fixup`. I hope this shows that the current MR's approach is solid enough for proxies later. A couple of things to note while reviewing, if you're wondering why it looks "hackish": * async props in localStorage etc are weird as hell. Defining an accessor bypasses the override, and stores a normal js prop, and getOwnPropertyDescriptor also retrieves the normal js prop "underneath" the one set by setItem, if available. But, that only applies to getOwnPropertyDescriptor. Retrieving the prop via normal getter looks at getItem first. Look at the "storage" tests starting in dom.js after line 577 (on my branch). * Other things making use of the "fixup" are the doc/window with props being from element lookups (or global props for window). But note that these are different from localStorage because they cannot *add* a new one, while localStorage can (e.g. localStorage.foobar = "1234" defining a new prop, but that can't happen on doc or window with the element lookup, it defines an actual jscript prop in those cases). They also don't override an existing js prop at all, so they only make use of the "fixup" but not the "override". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68049

Jacek Caban (＠jacek)

8:49 p.m.

On Wed Apr 17 19:23:20 2024 +0000, Gabriel Ivăncescu wrote:

...

I pushed new version with a small fix for arguments. I also rewrote the Typed Arrays and jscript proxy patches to make use of this. It's on my wine's branch `jscript-proxies`. The patches of note are cb615b32c4cd8cd9afed43bf1882df9139e6d365 and be63c5738979878a154b65e142983c9802feef4f. Note that they aren't split yet, since I keep rewriting them for now, it would make it too much wasted effort. But I don't think a full review of them is necessary, just the parts handling the props, right? Some things to note: * I got rid of PropDefineOverride and the "this" pointer for PropInvoke. * With respect to props, there are no special cases anymore for proxies, it's all handled in the props methods in the vtbl (from this MR). * Only one new method had to be added, `prop_fixup`. I hope this shows that the current MR's approach is solid enough for proxies later. A couple of things to note while reviewing, if you're wondering why it looks "hackish": * async props in localStorage etc are weird as hell. Defining an accessor bypasses the override, and stores a normal js prop, and getOwnPropertyDescriptor also retrieves the normal js prop "underneath" the one set by setItem, if available. But, that only applies to getOwnPropertyDescriptor. Retrieving the prop via normal getter looks at getItem first. Look at the "storage" tests starting in dom.js after line 577 (on my branch). * Other things making use of the "fixup" are the doc/window with props being from element lookups (or global props for window). But note that these are different from localStorage because they cannot *add* a new one, while localStorage can (e.g. localStorage.foobar = "1234" defining a new prop, but that can't happen on doc or window with the element lookup, it defines an actual jscript prop in those cases). They also don't override an existing js prop at all, so they only make use of the "fixup" but not the "override". The interface for functions looks suspicious and I think that's a result of special-casing them in a wrong place. If they are indeed JS functions, `dispex.c` should be able to just call it like any other function (and `proxy_disp_call` should not be needed). Then something like `ProxyFunction_call` could just call a `IJSDispatchHost` method of the proper object if it implements the required interface.

This `prop_fixup` sounds solvable by just using different override strategy in relevant code paths. In one case we may interpret such case differently than in the other, I'm sure there is a cleaner way for that. Did you consider allowing enums without callbacks to jscript? Ie. `IJSDispatchHost` enum could have an ID parameter and return the next one like `IDispatchEx` does? jscript could still additionally skip over properties if needed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68444

Gabriel Ivăncescu

8:49 p.m.

...

The interface for functions looks suspicious and I think that's a result of special-casing them in a wrong place. If they are indeed JS functions, `dispex.c` should be able to just call it like any other function (and `proxy_disp_call` should not be needed). Then something like `ProxyFunction_call` could just call a `IJSDispatchHost` method of the proper object if it implements the required interface.

But `proxy_disp_call` is not used for the functions, it's basically the analog for the builtin_info->call except for proxies. In fact, that's exactly where it is called from, the "call" method of JSCLASS_PROXY objects. It's for calling DISPID_VALUE mainly on objects (and constructors that aren't actual functions for instance, which is the majority of them). It is additionally used for calling PROP_PROXY props—these are not the functions, but rather calling a prop like for instance sessionStorage's props and so on. You know, like our builtins on jscript that aren't functions nor accessors. Those can be invoked too, although it will likely error out. In such case, the error is given by mshtml, which I found more extensible and cleaner so jscript doesn't have to worry about it (I don't know yet if some objects return something else, either). Plus, it's just one line of code re-using the same helper that's needed anyway… I could hardcode the error in jscript if you prefer it, but I'm not sure that buys us anything? `proxy_disp_call` is already used by the builtin_info->call anyway. If you want an example of an object that's **not** a function that still needs to be invoked: anchor element's DISPID_VALUE.

...

This `prop_fixup` sounds solvable by just using different override strategy in relevant code paths. In one case we may interpret such case differently than in the other, I'm sure there is a cleaner way for that.

Sorry, I don't quite understand what you mean here. I use different cases because of how native works, to pass existing and new tests. `prop_fixup` is basically similar to the PROP_PROTREF fixup (in fact, the PROTREF fixup is implemented with it too), except it's for props that may pop into existence or pop out of existence at any point. Can you be more specific about the other strategy please? And are you sure it's going to work in all cases? EDIT: Regardless, I do believe that since it's a new method added only for proxies, this MR should be fine as it is, right? I mean the remaining methods in this MR do seem fine unless I'm missing something...?

...

Did you consider allowing enums without callbacks to jscript? Ie. `IJSDispatchHost` enum could have an ID parameter and return the next one like `IDispatchEx` does? jscript could still additionally skip over properties if needed.

Yeah, I guess that can work too, the only thing jscript is doing right now is to just populate the props (with find_prop_name), similar to fill_props. It's probably unrelated to this MR though, I don't see how it affects it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68455

Jacek Caban (＠jacek)

8:52 p.m.

On Mon Apr 22 16:02:21 2024 +0000, Gabriel Ivăncescu wrote:

...

...
The interface for functions looks suspicious and I think that's a result of special-casing them in a wrong place. If they are indeed JS functions, `dispex.c` should be able to just call it like any other function (and `proxy_disp_call` should not be needed). Then something like `ProxyFunction_call` could just call a `IJSDispatchHost` method of the proper object if it implements the required interface. But `proxy_disp_call` is not used for the functions, it's basically the analog for the builtin_info->call except for proxies. In fact, that's exactly where it is called from, the "call" method of JSCLASS_PROXY objects. It's for calling DISPID_VALUE mainly on objects (and constructors that aren't actual functions for instance, which is the majority of them). It is additionally used for calling PROP_PROXY props—these are not the functions, but rather calling a prop like for instance sessionStorage's props and so on. You know, like our builtins on jscript that aren't functions nor accessors. Those can be invoked too, although it will likely error out. In such case, the error is given by mshtml, which I found more extensible and cleaner so jscript doesn't have to worry about it (I don't know yet if some objects return something else, either). Plus, it's just one line of code re-using the same helper that's needed anyway… I could hardcode the error in jscript if you prefer it, but I'm not sure that buys us anything? `proxy_disp_call` is already used by the builtin_info->call anyway. If you want an example of an object that's **not** a function that still needs to be invoked: anchor element's DISPID_VALUE. This `prop_fixup` sounds solvable by just using different override strategy in relevant code paths. In one case we may interpret such case differently than in the other, I'm sure there is a cleaner way for that. Sorry, I don't quite understand what you mean here. I use different cases because of how native works, to pass existing and new tests. `prop_fixup` is basically similar to the PROP_PROTREF fixup (in fact, the PROTREF fixup is implemented with it too), except it's for props that may pop into existence or pop out of existence at any point. Can you be more specific about the other strategy please? And are you sure it's going to work in all cases? EDIT: Regardless, I do believe that since it's a new method added only for proxies, this MR should be fine as it is, right? I mean the remaining methods in this MR do seem fine unless I'm missing something...? Did you consider allowing enums without callbacks to jscript? Ie. `IJSDispatchHost` enum could have an ID parameter and return the next one like `IDispatchEx` does? jscript could still additionally skip over properties if needed. Yeah, I guess that can work too, the only thing jscript is doing right now is to just populate the props (with find_prop_name), similar to fill_props. It's probably unrelated to this MR though, I don't see how it affects it. Scratch the reference to the implementation, let me put it differently: why do you need a separate interface for functions? They are always called on "this" object, so why not use `IJSDisatchHost` of "this" object? Its implementation may get a function descriptor from the ID. The only additional thing from function descriptor that you need earlier is IID, to make sure that the object implements required interface. You could just make that a part of function description. I think we already discussed this part years ago. Things like `get_dispex_for_hook` should not be needed in the first place.

`prop_fixup` is an implementation detail (not a pretty one) that shouldn't be a part of the interface in its current form. When you need to check if the prototype is still valid, you may just query that value's description, for example. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68778

Gabriel Ivăncescu

8:55 p.m.

On Wed Apr 24 22:01:05 2024 +0000, Jacek Caban wrote:

...

Scratch the reference to the implementation, let me put it differently: why do you need a separate interface for functions? They are always called on "this" object, so why not use `IJSDisatchHost` of "this" object? Its implementation may get a function descriptor from the ID. The only additional thing from function descriptor that you need earlier is IID, to make sure that the object implements required interface. You could just make that a part of function description. I think we already discussed this part years ago. Things like `get_dispex_for_hook` should not be needed in the first place. `prop_fixup` is an implementation detail (not a pretty one) that shouldn't be a part of the interface in its current form. When you need to check if the prototype is still valid, you may just query that value's description, for example. The "this" object can be anything. It doesn't have to be a mshtml object. So what happens then? That's why I store the entire function context along with the this object in the function interface. For the props, we do have a mshtml object (the one holding the prop), so we don't need that, hence why it's a different interface.

You did mention the IID. The only way I can see how this can work out is by duplicating the interface checks from mshtml into jscript. Is that actually a good idea? I'd rather not duplicate code if mshtml can already handle it. BTW, how do I even store the IID without the function context (in the function interface, not the proxy)? For `prop_fixup`, do you mean to use PropGetInfo? Or did I misunderstand? Honestly, I believe I tried that at first and something didn't work out, but that was awhile ago, maybe I was wrong or something changed since then. Not sure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68865

Gabriel Ivăncescu

8:55 p.m.

On Thu Apr 25 17:41:29 2024 +0000, Gabriel Ivăncescu wrote:

...

The "this" object can be anything. It doesn't have to be a mshtml object. So what happens then? That's why I store the entire function context along with the this object in the function interface. For the props, we do have a mshtml object (the one holding the prop), so we don't need that, hence why it's a different interface. You did mention the IID. The only way I can see how this can work out is by duplicating the interface checks from mshtml into jscript. Is that actually a good idea? I'd rather not duplicate code if mshtml can already handle it. BTW, how do I even store the IID without the function context (in the function interface, not the proxy)? For `prop_fixup`, do you mean to use PropGetInfo? Or did I misunderstand? Honestly, I believe I tried that at first and something didn't work out, but that was awhile ago, maybe I was wrong or something changed since then. Not sure. Actually wait nevermind, we can't use the IJSDispatchHost for functions, because functions can be completely detached from the object:

var a = document.createEvent;
a(...)

I remember I did try that at first and it was a dead end. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68866

Jacek Caban (＠jacek)

8:56 p.m.

On Thu Apr 25 17:44:33 2024 +0000, Gabriel Ivăncescu wrote:

...

Actually wait nevermind, we can't use the IJSDispatchHost for functions, because functions can be completely detached from the object: ```jscript var a = document.createEvent; a(...) ``` I remember I did try that at first and it was a dead end. If "this" object is not MSHTML object, then we know that it doesn't implement the required interface and we throw an appropriate error. The code from your example should fail, unless I'm missing something. It's really the same: it would get the default "this" handling, which means that "this" object doesn't implement the required interface. (That code could work in older compat modes, where functions were tied to object instances, but it's a very different semantics and not interesting for the interface).

For `IID`, it seems to me that the first step would be to replace `proxy_func_invoker` with just `IID`, you already have `DISPID` anyway. And yes, for `prop_fixup` I meant something like that. It may be more convenient to have an ID-based variant (instead of name lookup), possibly not, I'm not sure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68948

Gabriel Ivăncescu

8:56 p.m.

On Fri Apr 26 12:13:08 2024 +0000, Jacek Caban wrote:

...

If "this" object is not MSHTML object, then we know that it doesn't implement the required interface and we throw an appropriate error. The code from your example should fail, unless I'm missing something. It's really the same: it would get the default "this" handling, which means that "this" object doesn't implement the required interface. (That code could work in older compat modes, where functions were tied to object instances, but it's a very different semantics and not interesting for the interface). For `IID`, it seems to me that the first step would be to replace `proxy_func_invoker` with just `IID`, you already have `DISPID` anyway. And yes, for `prop_fixup` I meant something like that. It may be more convenient to have an ID-based variant (instead of name lookup), possibly not, I'm not sure. OK, I see what you mean with the IID. I didn't think of it, because I thought of delegating everything to mshtml (which already queries the IID and returns appropriate error) and making a generic interface for jscript so that it won't care of also checking it, but if you're ok with that I can try to squash the interfaces.

For the lookup, we need name-based lookup unfortunately because that's the whole point of the fixup (and override): such props may not have existed before, so what would the ID even be? That said, I'm not sure either of these have to do with this MR, I mean "prop_fixup" itself is not tied to the fixup on the mshtml side (which can have a different interface later), but it's for jscript code itself. Anything in particular wrong with the interfaces in this MR? Or something you'd rather have done differently? Even if we change PropFixup to PropGetInfo, I don't think it will change the jscript side for this MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_68958

Gabriel Ivăncescu

9:04 p.m.

On Fri Apr 26 15:52:42 2024 +0000, Gabriel Ivăncescu wrote:

...

OK, I see what you mean with the IID. I didn't think of it, because I thought of delegating everything to mshtml (which already queries the IID and returns appropriate error) and making a generic interface for jscript so that it won't care of also checking it, but if you're ok with that I can try to squash the interfaces. For the lookup, we need name-based lookup unfortunately because that's the whole point of the fixup (and override): such props may not have existed before, so what would the ID even be? That said, I'm not sure either of these have to do with this MR, I mean "prop_fixup" itself is not tied to the fixup on the mshtml side (which can have a different interface later), but it's for jscript code itself. Anything in particular wrong with the interfaces in this MR? Or something you'd rather have done differently? Even if we change PropFixup to PropGetInfo, I don't think it will change the jscript side for this MR. I pushed new version to `jscript-proxies` that should address the suggestions. But it didn't change anything in the jscript object vtbl (for props I mean, not those interacting with mshtml). So for this MR, nothing changed at all.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_69255

Jacek Caban (＠jacek)

9:09 p.m.

On Tue Apr 30 19:10:03 2024 +0000, Gabriel Ivăncescu wrote:

...

I pushed new version to `jscript-proxies` that should address the suggestions. But it didn't change anything in the jscript object vtbl (for props I mean, not those interacting with mshtml). So for this MR, nothing changed at all. That part looks better, thanks. FWIW, if QI itself would fit better in MSHTML, it could also be just passed back together with ID.

`PropOverride` still looks weird to have in the interface. Can't we use other functions for that? It's essentially a `GetPropInfo` combined with a getter call. For calling the other way around, forwarding MSHTML `IDispatchEx` to jscript looks reasonable, but do you have some tests confirming it? Another approach would be to have them keep doing 'IE8' thing and use something entirely separate for JS. An example of a test distinguishing those approaches would be div element's `IDispatchEx` being able to query properties of `Element` and `Object` methods, respond to defining new ones (on prototypes) and deleting ones, something like that. For enums, I didn't really mean a glue code, but to consider if we'd want to change it at the core. It would be great if you'd include the code required for proper external prototype support, that should answer a number of my questions. Generally, jscript guarantees that if a property was exposed from given object once, a property of that name will always have the same dispid, even if it's removed and redefined. This is partially to make sure that callers `IDispatch` dispid caching works, but it's probably more than we really need to guarantee there. Current jsdisp implementation depends on this strict behavior for correctness. That's what allows prototype reference to work like they do right now, but maintaining an invariant like this is not really convenient for MSHTML. It's not clear to me what's your plan here; you tweaked that for enum, but I'm afraid that for allowing MSHTML objects to be prototypes we may need to change it at the core. Anyway, it's probably better to talk with a prototype, since you already have it implemented. I'd be mostly interested with minimal testable case, like a single MSHTML object with required interface and implementation changes. Could you please port something like that into the branch?

...

But it didn't change anything in the jscript object vtbl (for props I mean, not those interacting with mshtml). So for this MR, nothing changed at all.

Well, you put some glue code instead of changing jscript, so nothing changed. I really don't review this MR can be reviewed of the context. Most of comments about the interface apply to this internal interface as well. For example, I hope there is a better way than 'fixup' callbacks (even if it's not technically in this MR, that's what this MR leads to). I think that the best way forward is to get the prototype into a better shape first. It would be great if you could get unneeded parts out of it. That includes typed arrays and a few other commits, I really hope that we can take care of MSHTML bindings first. Since we already started discussing it here, maybe it makes sense to push such stripped branch to this MR, rename it and mark as a draft? Or have a new draft, if you prefer? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_69971

Gabriel Ivăncescu

9:09 p.m.

...

`PropOverride` still looks weird to have in the interface. Can't we use other functions for that? It's essentially a `GetPropInfo` combined with a getter call.

Well the thing is it only applies to specific objects, and must return S_FALSE otherwise so we know it doesn't. Not sure how to fit that in existing functions without over-engineering them? I could make a function that returns a BOOL like `OverridesProps` but that's just replacing it with another…

...

For calling the other way around, forwarding MSHTML `IDispatchEx` to jscript looks reasonable, but do you have some tests confirming it? Another approach would be to have them keep doing 'IE8' thing and use something entirely separate for JS. An example of a test distinguishing those approaches would be div element's `IDispatchEx` being able to query properties of `Element` and `Object` methods, respond to defining new ones (on prototypes) and deleting ones, something like that.

For enums, I didn't really mean a glue code, but to consider if we'd want to change it at the core. It would be great if you'd include the code required for proper external prototype support, that should answer a number of my questions.

Generally, jscript guarantees that if a property was exposed from given object once, a property of that name will always have the same dispid, even if it's removed and redefined. This is partially to make sure that callers `IDispatch` dispid caching works, but it's probably more than we really need to guarantee there.

Current jsdisp implementation depends on this strict behavior for correctness. That's what allows prototype reference to work like they do right now, but maintaining an invariant like this is not really convenient for MSHTML. It's not clear to me what's your plan here; you tweaked that for enum, but I'm afraid that for allowing MSHTML objects to be prototypes we may need to change it at the core.

Not sure if we have a test (or how to test it?), but native **does not** use the fixed DISPIDs in IE9+ like in IE8 and below, which suggests that it does something similar to my approach. See `test_elem_GetIDsOfNames` in `tests/dom.c` comment; feel free to remove that check and watch it fail. Obviously I can't test for fixed DISPID that's why I skipped it… There is also `test_doc_obj` in `tests/events.c` with the comment and following test at `Navigate to a different document mode page`, where we test `importNode`'s DISPID, and it's not fixed. In my case we guarantee the DISPIDs are the same for the given name, just not across different objects, but on the same object. Isn't that enough? It is the same as jscript. Two jscript objects, even if they're the same object "type", will likely have different DISPIDs for the same name. The only thing we don't guarantee is their "fixed" DISPIDs but that's already the case on native in IE9+. I don't really understand what you mean by enums. Right now it's basically just doing what jscript does, filling the props, except instead of jscript builtins it's mshtml builtins (which end up as proxies, either functions, accessors or PROP_PROXY). It works the same way, there's zero changes needed for prototypes. We have a new method to deal with it because we have to bypass the normal `GetNextDispID` since that would call back into us, just like e.g. PropInvoke vs InvokeEx.

...

Anyway, it's probably better to talk with a prototype, since you already have it implemented. I'd be mostly interested with minimal testable case, like a single MSHTML object with required interface and implementation changes. Could you please port something like that into the branch?

Sorry, I don't understand what you mean here. Wouldn't special casing one object be worse, because it would include extra checks/code (generic is easier, it applies to all DispatchEx on mshtml side), **and** there's no guarantee that it would cover all cases, which is the main point isn't it? If we didn't have objects like `localStorage` then all this override/fixup wouldn't have been needed. Tbh I'm not even sure how to begin special casing just one object.

...

Well, you put some glue code instead of changing jscript, so nothing changed. I really don't review this MR can be reviewed of the context. Most of comments about the interface apply to this internal interface as well. For example, I hope there is a better way than 'fixup' callbacks (even if it's not technically in this MR, that's what this MR leads to).

Unfortunately I can't see a better way, since there's no way even for mshtml code to know when some underlying storage changed for those objects (no callbacks or anything). We already fixup the PROP_PROTREFs so I thought reusing that would make it more acceptable. The other concerns, like forwarding calls to jscript, DISPIDs, and so on, I don't see how they're relevant to this MR? I prefer doing it a bit at a time instead of all at once.

...

I think that the best way forward is to get the prototype into a better shape first. It would be great if you could get unneeded parts out of it. That includes typed arrays and a few other commits, I really hope that we can take care of MSHTML bindings first. Since we already started discussing it here, maybe it makes sense to push such stripped branch to this MR, rename it and mark as a draft? Or have a new draft, if you prefer?

I can, but this is a lot more work than Typed Arrays, so I wanted to get those in first so it doesn't keep piling up. Note that I haven't split it up yet, and I want to get it in acceptable shape somewhat first because re-writing it constantly if it's split up becomes a nightmare. Though Typed Arrays don't really affect this much, they do show that they are enough for the current jscript prop methods in the vtbl, isn't that enough for this MR? Are you sure you don't want to review this MR as-is so I can get those in first? "fixup" may not be good, but it's not part of this MR as noted, and so if we find a better way later it wouldn't affect this MR. The other methods are totally justified for non-mshtml cases so we need them regardless of what happens with mshtml bindings. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70055

Jacek Caban (＠jacek)

9:09 p.m.

...

Not sure if we have a test (or how to test it?)

I didn't talk about testing it, but your observations confirm my suspicious. Now imagine that we have `pobj` like that, with async properties not guarantying dispid stability, which initially has `async1` prop. Now consider this: ``` function f() {} f.prototype = pobj; var o = new f(); x = o.async1; // some other code; meantime async1 disappears, async2 appears and then a new async1 appears with a different dispid x = o.async1; // problem: now prototype ref points to wrong property and simple name dispid or name validation will not be enough ``` I expect the above to be a problem if you base prototypes on something like this MR, so I'm curious what you did about it. The test I asked was about something different, to show how `IDispatchEx should behave. That requires prototypes to test: ``` div = document.getElementById("divid"); Object.prototype.x = 1; // verify that div IDispatchEx can query x Element.prototype.y = 2; // verify that div IDispatchEx can query y detele Object.prototype.x; // ... ``` Do you have a test like that?

...

The other concerns, like forwarding calls to jscript, DISPIDs, and so on, I don't see how they're relevant to this MR? I prefer doing it a bit at a time instead of all at once.

The test above may be a bit of corner case, but more generally forwarding calls to jscript is by no means a corner case, it affects a pretty core part of the design and this MR is a part of its implementation. I'm not very interested in corner cases at this point, but I'm interested in making sure we don't end up with ad-hoc design mess (like the code in Proton). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70088

Gabriel Ivăncescu

9:10 p.m.

On Fri May 10 00:16:53 2024 +0000, Jacek Caban wrote:

...

...
Not sure if we have a test (or how to test it?) I didn't talk about testing it, but your observations confirm my suspicious. Now imagine that we have `pobj` like that, with async properties not guarantying dispid stability, which initially has `async1` prop. Now consider this:
function f() {}
f.prototype = pobj;
var o = new f();
x = o.async1;
// some other code; meantime async1 disappears, async2 appears and then
a new async1 appears  with a different dispid
x = o.async1; // problem: now prototype ref points to wrong property and
simple name dispid or name validation will not be enough
I expect the above to be a problem if you base prototypes on something like this MR, so I'm curious what you did about it. The test I asked was about something different, to show how `IDispatchEx should behave. That requires prototypes to test: ``` div = document.getElementById("divid"); Object.prototype.x = 1; // verify that div IDispatchEx can query x Element.prototype.y = 2; // verify that div IDispatchEx can query y detele Object.prototype.x; // ... ``` Do you have a test like that?

...
The other concerns, like forwarding calls to jscript, DISPIDs, and so on, I don't see how they're relevant to this MR? I prefer doing it a bit at a time instead of all at once. The test above may be a bit of corner case, but more generally forwarding calls to jscript is by no means a corner case, it affects a pretty core part of the design and this MR is a part of its implementation. I'm not very interested in corner cases at this point, but I'm interested in making sure we don't end up with ad-hoc design mess (like the code in Proton). I tried hard to reproduce what you meant but it doesn't seem to be possible? I mean, if o.async1 disappears, it doesn't mean the DISPID disappears. It just becomes PROP_DELETED on jscript or equivalent. Obviously it disappears from the jscript code's perspective, but not when querying the DISPID, the name is still "stored" in the object's props. Am I missing something?

I'm just using jscript's facility here. If you mean the "override" rather than the "fixup", then it's done on *existing props* so still have the same DISPID, although it's looked up via name on mshtml side but that's purely implementation detail, since mshtml doesn't know about jscript's DISPID (I guess it could be possible to obtain the name from DISPID via GetMemberName or the like but why complicate it when we can pass the name?). In short: the prop "fixup" doesn't fixup the name, but rather the prop's value/contents (or deletes the prop). But deleted props don't delete the name—and DISPIDs are associated with the name, not the contents of the prop. So: ```jscript x = o.async1 // ... x = o.async1 ``` First one let's say creates the async1 prop. Then it gets deleted on the file. jscript doesn't know this of course. Next time we obtain it, it's actually deleted, so it returns "undefined". But note that this is done in the getter, which already has the DISPID—the same DISPID as previously. Unless I'm missing something? Maybe I'm testing it wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70152

Jacek Caban (＠jacek)

9:10 p.m.

On Fri May 10 17:20:07 2024 +0000, Gabriel Ivăncescu wrote:

...

I tried hard to reproduce what you meant but it doesn't seem to be possible? I mean, if o.async1 disappears, it doesn't mean the DISPID disappears. It just becomes PROP_DELETED on jscript or equivalent. Obviously it disappears from the jscript code's perspective, but not when querying the DISPID, the name is still "stored" in the object's props. Am I missing something? I'm just using jscript's facility here. If you mean the "override" rather than the "fixup", then it's done on *existing props* so still have the same DISPID, although it's looked up via name on mshtml side but that's purely implementation detail, since mshtml doesn't know about jscript's DISPID (I guess it could be possible to obtain the name from DISPID via GetMemberName or the like but why complicate it when we can pass the name?). In short: the prop "fixup" doesn't fixup the name, but rather the prop's value/contents (or deletes the prop). But deleted props don't delete the name—and DISPIDs are associated with the name, not the contents of the prop. So: ```jscript x = o.async1 // ... x = o.async1 ``` First one let's say creates the async1 prop. Then it gets deleted on the file. jscript doesn't know this of course. Next time we obtain it, it's actually deleted, so it returns "undefined". But note that this is done in the getter, which already has the DISPID—the same DISPID as previously. Unless I'm missing something? Maybe I'm testing it wrong. `o`'s dispid points to a protocol reference, which references the prototype's property by dispid. The prototype reference is never removed in my example (only the property in the prototype, but `o` wouldn't know about it even if it wasn't an "async" property), it would still forward to the prototype, but potentially using a wrong dispid now.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70153

Jacek Caban (＠jacek)

9:10 p.m.

...

`o`'s dispid points to a protocol reference

I meant prototype reference. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70154

Gabriel Ivăncescu

9:10 p.m.

On Fri May 10 19:38:37 2024 +0000, Jacek Caban wrote:

...

...
`o`'s dispid points to a protocol reference I meant prototype reference. Wait so let's break it down a bit. Are you talking about the DISPID on mshtml side (the "internal" prop on mshtml, e.g. dynamic prop or whatever), or the DISPID on jscript side?

The DISPID on mshtml side is opaque to jscript, it's part of the proxy function/accessor/PROP_PROXY, but jscript doesn't really need to know about it. You can think of it as an opaque `void *context` so it shouldn't interfere with prototype lookup at all. Now if you mean the DISPID on jscript side, I can't reproduce what you describe. Let's say `o` has PROP_PROTREF that points to the async prop, let's say it's 42. The backing file then removes the prop on the prototype, so next time we "fixup" the prop we turn it (on the prototype) into PROP_DELETED. The PROP_PROTREF should then be fixed up whenever accessed just like normally via our fix_protref_prop, as we lookup where `42` points to and it's a PROP_DELETED now, so the PROP_PROTREF gets turned into a PROP_DELETED itself (except I've now moved it into a vtbl function so that proxy objects can override it). Note that prop_fixup on proxy objects should forward to the standard `dispex_prop_fixup` where necessary (to fixup PROTREF as mentioned), so this should be handled already. That PROTREF fixup was already a thing before this patch, actually it's a thing before this entire MR… It's not exclusive to async props. If I'm still misunderstanding, can you please provide a simple case that actually fails, no matter how hacky it is, I don't mind. It doesn't have to even use a mshtml object, just hack something so it "acts" like one? (actually that's how I've been trying to reproduce this, to no avail) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70174

Jacek Caban (＠jacek)

9:11 p.m.

On Sat May 11 17:42:25 2024 +0000, Gabriel Ivăncescu wrote:

...

Wait so let's break it down a bit. Are you talking about the DISPID on mshtml side (the "internal" prop on mshtml, e.g. dynamic prop or whatever), or the DISPID on jscript side? The DISPID on mshtml side is opaque to jscript, it's part of the proxy function/accessor/PROP_PROXY, but jscript doesn't really need to know about it. You can think of it as an opaque `void *context` so it shouldn't interfere with prototype lookup at all. Now if you mean the DISPID on jscript side, I can't reproduce what you describe. Let's say `o` has PROP_PROTREF that points to the async prop, let's say it's 42. The backing file then removes the prop on the prototype, so next time we "fixup" the prop we turn it (on the prototype) into PROP_DELETED. The PROP_PROTREF should then be fixed up whenever accessed just like normally via our fix_protref_prop, as we lookup where `42` points to and it's a PROP_DELETED now, so the PROP_PROTREF gets turned into a PROP_DELETED itself (except I've now moved it into a vtbl function so that proxy objects can override it). Note that prop_fixup on proxy objects should forward to the standard `dispex_prop_fixup` where necessary (to fixup PROTREF as mentioned), so this should be handled already. That PROTREF fixup was already a thing before this patch, actually it's a thing before this entire MR… It's not exclusive to async props. If I'm still misunderstanding, can you please provide a simple case that actually fails, no matter how hacky it is, I don't mind. It doesn't have to even use a mshtml object, just hack something so it "acts" like one? (actually that's how I've been trying to reproduce this, to no avail) Ah, right, shadow dispids would take care of it. The `PROP_PROXY` thing is confusing to me, I'm not yet sure what to think about it. I thought that the point of this MR is to allow external properties to not need `dispex_prop_t` (just like jscript internal indexed props). I can see how some things are easier this way (through I feel like the same would be true for other parts if we didn't allocate those). It may be useful to allow "shadowing" properties for some cases, but for that a flag in `dispex_prop_t` could be enough, I'm not sure it needs a new property type. Did you consider something like that? Is it not feasible?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70231

Gabriel Ivăncescu

9:11 p.m.

On Mon May 13 09:57:45 2024 +0000, Jacek Caban wrote:

...

Ah, right, shadow dispids would take care of it. The `PROP_PROXY` thing is confusing to me, I'm not yet sure what to think about it. I thought that the point of this MR is to allow external properties to not need `dispex_prop_t` (just like jscript internal indexed props). I can see how some things are easier this way (through I feel like the same would be true for other parts if we didn't allocate those). It may be useful to allow "shadowing" properties for some cases, but for that a flag in `dispex_prop_t` could be enough, I'm not sure it needs a new property type. Did you consider something like that? Is it not feasible? We have to use `dispex_prop_t` because we need the name and DISPID for them in this case, they're not just indices.

PROP_PROXY is simply a prop that delegates to mshtml side, it's very similar to PROP_BUILTIN except the "builtin" here is on mshtml, so we just store the necessary info to access it (mshtml DISPID, basically). Note that it is *not* for functions or accessors, it's for the other props (custom props for instance), just like PROP_BUILTIN. There's other ways of implementing it but this is the most straightforward, I think, since it simply forwards. I'm not sure how to implement these custom props *without* dispex_prop_t because, again, we need a name and that's what dispex_prop_t facility is for. I could make an exception for indexed mshtml props (custom props that are just indices), but that needs special-casing and I don't think it's good to complicate this initial design with it. It can come later, since it doesn't affect functionality, just an optimization. BTW for "shadowing props". PROP_PROXY props aren't special in the sense of shadowing other props, they're props just like PROP_BUILTIN for example. The "overriding" props (in localStorage for instance) are special, though, but that's not related to PROP_PROXY directly; they override any prop, not just PROP_PROXY. I think it helps if you think of PROP_PROXY just like PROP_BUILTIN except the builtin code is in mshtml, but that's it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70272

Jacek Caban (＠jacek)

9:11 p.m.

...

We have to use `dispex_prop_t` because we need the name and DISPID for them in this case, they're not just indices.

They could potentially be just indices, the association between name and ID may be easily done on MSHTML side (it's already doing that now).

...

Note that it is _not_ for functions or accessors, it's for the other props (custom props for instance), just like PROP_BUILTIN.

I think I can live with it, but note that the requirement to create a function object instance for all MSHTML builtin functions that are used is not exactly optimal. It would be possible to optimize that in the future, hopefully without full rewrite of the interface. Just like we do for jscript builtin functions.

...

I think it helps if you think of PROP_PROXY just like PROP_BUILTIN except the builtin code is in mshtml, but that's it.

Sure, although... it's by no means something for now, but I could imagine getting rid of `PROP_BUILTIN` and use the same 'external' properties for them instead, just like other external props (MSHTML and indexed props in the current proposal). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70279

Gabriel Ivăncescu

9:11 p.m.

...

They could potentially be just indices, the association between name and ID may be easily done on MSHTML side (it's already doing that now).

I don't think I follow. The name is used for jscript prop lookup, not mshtml. We absolutely need it for hash and standard name lookup. It's not an internal thing. PROP_PROXY doesn't care what name it has, it only stores the DISPID (index basically) to access it from mshtml. But looking it up **does** require name, because the prop has a name, so we do need dispex_prop_t, just like builtins. To be fair I'm talking about **being accessed by name via jscript**, nothing to do with mshtml internals. Think of window props, for example. I'm not talking about simple dynamic props on the window, which can be stored in jscript as simple values (and will be, later though, not in my branch yet, as I said I don't want to overcomplicate it). Instead, think of the other "global_props" (from mshtml's meaning of it) on the window, such as other scripts, elements, etc. Those need PROP_PROXY to get accessed…

...

I think I can live with it, but note that the requirement to create a function object instance for all MSHTML builtin functions that are used is not exactly optimal. It would be possible to optimize that in the future, hopefully without full rewrite of the interface. Just like we do for jscript builtin functions.

We can save that for later, but I believe it's as optimal as builtins right now. That is, they get allocated on demand (when needed), but otherwise we need them as functions, because even builtins had to be converted from builtins into actual functions at some point. Only the "value prop" builtins still remain as such (it's same with PROP_PROXY).

...

Sure, although... it's by no means something for now, but I could imagine getting rid of `PROP_BUILTIN` and use the same 'external' properties for them instead, just like other external props (MSHTML and indexed props in the current proposal).

How would that work? Builtins have a name, so we can't just use the same as indexed props because such name needs to have a DISPID mapping to it and be looked up by hash. We could use an extra layer of mapping for builtins of course, but I think that's overkill and likely slower (instead of just one hash lookup, now it's two lookups). It also complicates it because, for instance, builtins can be deleted, and then we have to actually implement every prop behavior on them instead of relying on dispex_prop_t facilities. Right now if they get deleted, the prop remains as PROP_DELETED and so can be easily replaced by something else. But yeah, maybe it's worth it way later though, we'll need measurements I guess. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_70308

568

Age (days ago)

607

Last active (days ago)

List overview

Download

31 comments

2 participants

participants (2)

Gabriel Ivăncescu
Jacek Caban (＠jacek)

[PATCH v3 0/11] MR5444: jscript: Do not allocate dispex_prop_t for indexed props.

tags

participants (2)