From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/jscript/jsutils.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/dlls/jscript/jsutils.c b/dlls/jscript/jsutils.c index 2c6516e06bb..1d6fa06a993 100644 --- a/dlls/jscript/jsutils.c +++ b/dlls/jscript/jsutils.c @@ -293,13 +293,8 @@ HRESULT variant_to_jsval(script_ctx_t *ctx, VARIANT *var, jsval_t *r) hres = IWineJSDispatchHost_GetJSDispatch(disp_host, &jsdisp_iface); IWineJSDispatchHost_Release(disp_host); if(SUCCEEDED(hres)) { - jsdisp_t *jsdisp = as_jsdisp((IDispatch *)jsdisp_iface); - if(jsdisp->ctx == ctx) { - *r = jsval_obj(jsdisp); - return S_OK; - }else { - jsdisp_release(jsdisp); - } + *r = jsval_obj(as_jsdisp((IDispatch *)jsdisp_iface)); + return S_OK; } } }
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/mshtml/nsevents.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/dlls/mshtml/nsevents.c b/dlls/mshtml/nsevents.c index 5d52cf4f17a..3fb882530f7 100644 --- a/dlls/mshtml/nsevents.c +++ b/dlls/mshtml/nsevents.c @@ -441,8 +441,12 @@ static nsresult handle_htmlevent(HTMLDocumentNode *doc, nsIDOMEvent *nsevent) }else { hres = get_node(nsnode, TRUE, &node); nsIDOMNode_Release(nsnode); - if(FAILED(hres) || !node->doc->script_global) + if(FAILED(hres)) + return NS_OK; + if(!node->doc->script_global) { + IHTMLDOMNode_Release(&node->IHTMLDOMNode_iface); return NS_OK; + } target = &node->event_target; }
Jacek Caban (@jacek) commented about dlls/jscript/jsutils.c:
hres = IWineJSDispatchHost_GetJSDispatch(disp_host, &jsdisp_iface); IWineJSDispatchHost_Release(disp_host); if(SUCCEEDED(hres)) {
jsdisp_t *jsdisp = as_jsdisp((IDispatch *)jsdisp_iface);if(jsdisp->ctx == ctx) {*r = jsval_obj(jsdisp);return S_OK;}else {jsdisp_release(jsdisp);}
*r = jsval_obj(as_jsdisp((IDispatch *)jsdisp_iface));return S_OK;
This obviously needs a test.
While we do want to remove the check, it was originally intended to preserve assumptions about the caller and callee sharing the same context, similar to how we check context in places like `disp_call`. For example, one such assumption is that we don’t propagate the caller’s context when making a call to another context. This will need to be fixed first, but the tricky question is: what else are we missing?
On Wed Feb 26 10:46:52 2025 +0000, Jacek Caban wrote:
This obviously needs a test. While we do want to remove the check, it was originally intended to preserve assumptions about the caller and callee sharing the same context, similar to how we check context in places like `disp_call`. For example, one such assumption is that we don’t propagate the caller’s context when making a call to another context. This will need to be fixed first, but the tricky question is: what else are we missing?
I can try to add a test, but it's obviously wrong the way it is now (see below). Either way, I don't think the fix for lack of context propagation should be done here, it feels like it's unrelated and a bit of a hack, even if it's missing (the propagation).
What this patch fixes is rather that on a differing context, we don't end up using the jsdisp at all now, instead we use the mshtml's Dispatch instead. This results in inconsistency and some checks not holding even while they should on differing contexts (e.g. the non-function constructors being treated as generic disp objects). Maybe I can add a test with that to show you what I mean.
I don't think it's right to *ever* use the mshtml's disp in scripts, except when forwarding from the jsdisp, or from C/COM code, no matter the ctx.
I can try to add a test, but it's obviously wrong the way it is now (see below).
Sure, the direction is clear. In this case it's not a question what Windows does, but to make sure Wine works properly and avoid regressions in the future.
Either way, I don't think the fix for lack of context propagation should be done here, it feels like it's unrelated and a bit of a hack, even if it's missing (the propagation).
Well, `IDispatchEx` code path gets it right, so it would be a regression.
Jacek Caban commented on a discussion on dlls/jscript/jsutils.c: https://gitlab.winehq.org/wine/wine/-/merge_requests/7425#note_96014
hres =IWineJSDispatchHost_GetJSDispatch(disp_host, &jsdisp_iface); IWineJSDispatchHost_Release(disp_host); if(SUCCEEDED(hres)) {
jsdisp_t *jsdisp = as_jsdisp((IDispatch*)jsdisp_iface);
if(jsdisp->ctx == ctx) {*r = jsval_obj(jsdisp);return S_OK;}else {jsdisp_release(jsdisp);}
*r = jsval_obj(as_jsdisp((IDispatch*)jsdisp_iface));
return S_OK;I can try to add a test, but it's obviously wrong the way it is now (see below).
Sure, the direction is clear. In this case it's not a question what Windows does, but to make sure Wine works properly and avoid regressions in the future.
Either way, I don't think the fix for lack of context propagation should be done here, it feels like it's unrelated and a bit of a hack, even if it's missing (the propagation).
Well, `IDispatchEx` code path gets it right, so it would be a regression.
Wait, how would it be a regression? The important parts (that end up in Invoke) will still end up through the IDispatch since they have different contexts. I don't see why mshtml objects (that should be jsdisps) should be treated differently than "native" jscript objects?
It's not like making them jsdisps will forcefully ignore the ctx checks and always end up in the jsdisp code path. Those will still be done where necessary, just like for generic jscript objects.
On Wed Feb 26 15:59:27 2025 +0000, Gabriel Ivăncescu wrote:
Jacek Caban commented on a discussion on dlls/jscript/jsutils.c: https://gitlab.winehq.org/wine/wine/-/merge_requests/7425#note_96014
hres =IWineJSDispatchHost_GetJSDispatch(disp_host, &jsdisp_iface); IWineJSDispatchHost_Release(disp_host); if(SUCCEEDED(hres)) {
jsdisp_t *jsdisp = as_jsdisp((IDispatch*)jsdisp_iface);
if(jsdisp->ctx == ctx) {*r = jsval_obj(jsdisp);return S_OK;}else {jsdisp_release(jsdisp);}
*r = jsval_obj(as_jsdisp((IDispatch*)jsdisp_iface));
return S_OK;I can try to add a test, but it's obviously wrong the way it is now (see below).
Sure, the direction is clear. In this case it's not a question what Windows does, but to make sure Wine works properly and avoid regressions in the future.
Either way, I don't think the fix for lack of context propagation should be done here, it feels like it's unrelated and a bit of a hack, even if it's missing (the propagation).
Well, `IDispatchEx` code path gets it right, so it would be a regression.
Wait, how would it be a regression? The important parts (that end up in Invoke) will still end up through the IDispatch since they have different contexts. I don't see why mshtml objects (that should be jsdisps) should be treated differently than "native" jscript objects? It's not like making them jsdisps will forcefully ignore the ctx checks and always end up in the jsdisp code path. Those will still be done where necessary, just like for generic jscript objects.
Actually I found another problem, while adding these tests (it's a fragile issue that the tests somehow triggered, not sure why exactly). Obviously I'll have to fix it before adding the tests since otherwise existing test will fail.
The failure is in `window_open_self_test` on the `iframe.contentWindow === iframe_window` check. So what happens is that a navigation creates a new jsdisp, new ctx and all. But same outer window and it's supposed to be the same.
Obviously the jsdisp needs to be tied, in theory, to the outer window, but it's not that simple, because script ctx will also be different. This issue is probably avoided now because we just use the outer window's Dispatch, but that's not right either (any code path that relies on jsdisp will fail, since outer window will be treated like a "host object disp" instead of a jsdisp). The simplest case is e.g. Object.prototype.toString which will return [object Object] by *design* and not due to a FIXME. I added such test now.
On my Proton patches from last year, I had to deal with this specifically as well. What I did was re-use the existing jsdisp from the older inner window, if any, when updating it to a new one.
But on top of that I had to add code to deal with this migration and to move it from one context to another (on jscript side), while unlinking all the props, which basically reset the jsdisp while keeping its memory address. I think I'll have to do something similar now.
Good thing I caught it now, might explain some weird things I saw with complex iframes and navigations.
But on top of that I had to add code to deal with this migration and to move it from one context to another (on jscript side), while unlinking all the props, which basically reset the jsdisp while keeping its memory address. I think I'll have to do something similar now.
That sounds ugly, I hope we can avoid that. If it's just about the comparison, there are things like `IObjectIdentity`, I'm not sure without a closer look.
On Wed Feb 26 22:09:23 2025 +0000, Jacek Caban wrote:
But on top of that I had to add code to deal with this migration and
to move it from one context to another (on jscript side), while unlinking all the props, which basically reset the jsdisp while keeping its memory address. I think I'll have to do something similar now. That sounds ugly, I hope we can avoid that. If it's just about the comparison, there are things like `IObjectIdentity`, I'm not sure without a closer look.
Sorry for the delay, I had an emergency to take care of today.
`IObjectIdentity` isn't going to work, the problem itself isn't that we get a different jsdisp by itself. `disp_cmp` already checks for the host object, in this particular case GetOuterDispatch results in the outer window.
The problem is that the old iframe's inner window is detached on navigation. Thus, GetOuterDispatch will return the inner window's disp instead of the outer window, so it will mismatch. This isn't a problem if we "migrate" the jsdisp though, but that's not pretty as you said.
I remember I had a patch awhile ago to deal with this issue (and add tests to confirm it) by storing strong (cyclic) ref between inner and outer windows, maybe I need to revive it. But that one was also "not very pretty" and I had to drop it.
At this point I don't know what to do anymore with this particular problem, it keeps popping up and I can't find a "clean" solution. Maybe I'm missing something, but should I revive the outer window<->inner window cyclic ref patch again?
Anyway I think that patch is better (in functionality/correctness) than the jsdisp migration, and since both aren't pretty I guess it's a better contender.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jacek Caban (＠jacek)