On Wed Jun 5 23:33:45 2024 +0000, Jacek Caban wrote:
> Setting the timeout on the socket is quite expensive (requires a
> wineserver call). Maybe we could store the last set timeout separate
> from configure timeout and update socket timeout only when they don't match?
Would this proposal be only for `read_more_data` or in the broader scope of any call to `NETCON_set_timeout`?
If it is for the latter, it could be tracked in `netconn_t`. I can add that into a separate merge.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3518#note_72609
On Wed Jun 5 23:33:45 2024 +0000, Jacek Caban wrote:
> Do we still need it? I think that with implementation in both
> `set_global_option` and `INET_SetOption`, we could simply remove those
> cases from `InternetSetOptionW` fallback.
If we can assume that `vtbl->SetOption` is guaranteed to handle its own implementation of each timeout (or fallback to `INET_SetOption`), I could see dropping these cases from `InternetSetOptionW`.
I'll amend those changes.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3518#note_72608
> Well, I expect them to behave like JS functions. If it were a JS function, it would check to see if the object type matches, and then do its thing. If you think of it like this, it wouldn't make much sense for it to use another object type's function.
I'm not talking about another objects' functions, I'm talking about separate objects having the same prototypes in JS sense but being otherwise separate implementations of the same interfaces in C.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72607
On Thu Jun 6 19:05:56 2024 +0000, Jacek Caban wrote:
> > In such case, instead of failing, it would call the other function
> that was looked up, since it matches in IID and DISPID. It won't crash,
> but I don't think it would be quite correct, either.
> That's the whole point, the reason it's safe and I don't see why you
> think it's incorrect. BTW, IID is intentionally abstract in my proposal;
> a bit closer to the actual meaning would be to use something like
> prototype ID once we have those (interface ID is just already available
> and close enough).
> > Continuing the above, IMO the following would make it the most robust:
> we look it up as mentioned from the func_info_t's dispid and IID, and
> then compare to see if it's the same `func_info_t` pointer.
> Sure, but it can't compare `func_info_t`s by a pointer, it would need to
> compare IID or something like that anyway. So in your proposal, the
> opaque pointer serves as (ID,IID) carrier, nothing else.
> > BTW, is there a reason you renamed `IWineJSDispatchHost` to just
> `IJSDispatchHost` when it's Wine's own interface? I thought it's better
> to avoid ambiguity with a native interface, especially if we're to
> extend it (obviously we can't touch native interfaces), makes it a lot
> more obvious.
> It's all Wine code, I don't see how putting "wine" in random places
> makes it any cleaner, but it's not a strong preference and I won't
> insist either way.
Well, I expect them to behave like JS functions. If it were a JS
function, it would check to see if the object type matches, and then do
its thing. If you think of it like this, it wouldn't make much sense for
it to use another object type's function.
As for Wine prefix, we typically have that already for our own custom
interfaces in mshtml, and even though it's all Wine code, it makes it
clearer for someone unfamiliar to understand that the interface is not
something from Windows—so they can find its documentation/code in Wine
code with grep instead of a Google search, and can freely change them
around, etc.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72606
> In such case, instead of failing, it would call the other function that was looked up, since it matches in IID and DISPID. It won't crash, but I don't think it would be quite correct, either.
That's the whole point, the reason it's safe and I don't see why you think it's incorrect. BTW, IID is intentionally abstract in my proposal; a bit closer to the actual meaning would be to use something like prototype ID once we have those (interface ID is just already available and close enough).
> Continuing the above, IMO the following would make it the most robust: we look it up as mentioned from the func_info_t's dispid and IID, and then compare to see if it's the same `func_info_t` pointer.
Sure, but it can't compare `func_info_t`s by a pointer, it would need to compare IID or something like that anyway. So in your proposal, the opaque pointer serves as (ID,IID) carrier, nothing else.
> BTW, is there a reason you renamed `IWineJSDispatchHost` to just `IJSDispatchHost` when it's Wine's own interface? I thought it's better to avoid ambiguity with a native interface, especially if we're to extend it (obviously we can't touch native interfaces), makes it a lot more obvious.
It's all Wine code, I don't see how putting "wine" in random places makes it any cleaner, but it's not a strong preference and I won't insist either way.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72605
On Thu Jun 6 15:55:43 2024 +0000, Gabriel Ivăncescu wrote:
> Actually you don't even need to query IID anymore. Just look up the
> function by dispid, and then check if it's same func_info_t, that's enough.
BTW, is there a reason you renamed `IWineJSDispatchHost` to just `IJSDispatchHost` when it's Wine's own interface? I thought it's better to avoid ambiguity with a native interface, especially if we're to extend it (obviously we can't touch native interfaces), makes it a lot more obvious.
And to add to what I said above, what I meant with "retrieving a potentially different function" is mostly theoretical, because as you said current mshtml likely has only one struct layout per interface. **But** if we were to assume this is not the case (addressing the original concern), then two objects could have same IID but different struct layouts and different functions. In such case, instead of failing, it would call the other function that was looked up, since it matches in IID and DISPID. It won't crash, but I don't think it would be quite correct, either. And it still makes the code simpler anyway.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72601
--
v3: winegstreamer: Request the new transform output format explicitly.
winegstreamer: Use a caps to store the desired output format.
winegstreamer: Only report format changes when frontend supports it.
winegstreamer: Rename allow_size_change to allow_format_change.
winegstreamer: Enforce default stride presence in the video processor.
winegstreamer: Enforce default stride value in the video decoder.
winegstreamer: Allow to clear video decoder input/output types.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5732
This reduce the chance of buffer allocating issue for Kirikiri2 games(eg. Super Naughty Maid) when the game is trying to play videos.
--
v3: qasf/dmowrapper: Sync Stop() and Receive() for dmo wrapper filter.
qasf/dmowrapper: Return VFW_E_WRONG_STATE in dmo_wrapper_sink_Receive.
qasf/dmowrapper: Allocate output samples before calling ProcessInput().
qasf/dmowrapper: Return failure in dmo_wrapper_sink_Receive if samples allocating fails.
qasf/dmowrapper: Introduce release_output_samples.
qasf/dmowrapper: Introduce get_output_samples.
qasf/tests: Add more tests for dmo_wrapper_sink_Receive.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
On Thu Jun 6 15:51:53 2024 +0000, Gabriel Ivăncescu wrote:
> > But then you assume that all objects implementing the interface will
> use the same struct layout for that. I can't think of an example inside
> MSHTML where it's currently not the case, but that's not generally a
> promise that QI gives.
> In theory, there's a similar concern when looking up the function via
> IID and DISPID, specifically, that it might find a different function.
> In practice as you said it probably doesn't happen but…
> > So what's the point of it then? Avoiding one argument in a call?
> Continuing the above, IMO the following would make it the most robust:
> we look it up as mentioned from the func_info_t's dispid and IID, and
> then compare to see if it's the same `func_info_t` pointer. If not,
> return E_UNEXPECTED since it's the wrong function. And yes, one less
> argument to keep track of, but also one less thing to keep track of (in
> the ProxyFunction for instance, and everywhere else).
> > I think that you misinterprets your observations. Here is a test
> proving that currentStyle should have a setter that throws an error
> (unless used for modifiable style object):
> Okay, thanks for the test, I guess there's more at play here indeed.
Actually you don't even need to query IID anymore. Just look up the function by dispid, and then check if it's same func_info_t, that's enough.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72595
Didn't notice that it was failing until I did the same for winemac with https://gitlab.winehq.org/wine/wine/-/merge_requests/5798. It was silently falling back to copies while winemac can't do that.
--
v3: gdi.exe16: Fix some incorrect usage of NtGdiDdDDICreateDCFromMemory.
winex11: Fix some incorrect usage of NtGdiDdDDICreateDCFromMemory.
winex11: Wrap more window surface formats with NtGdiDdDDICreateDCFromMemory.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5799
> But then you assume that all objects implementing the interface will use the same struct layout for that. I can't think of an example inside MSHTML where it's currently not the case, but that's not generally a promise that QI gives.
In theory, there's a similar concern when looking up the function via IID and DISPID, specifically, that it might find a different function. In practice as you said it probably doesn't happen but…
> So what's the point of it then? Avoiding one argument in a call?
Continuing the above, IMO the following would make it the most robust: we look it up as mentioned from the func_info_t's dispid and IID, and then compare to see if it's the same `func_info_t` pointer. If not, return E_UNEXPECTED since it's the wrong function. And yes, one less argument to keep track of, but also one less thing to keep track of (in the ProxyFunction for instance, and everywhere else).
> I think that you misinterprets your observations. Here is a test proving that currentStyle should have a setter that throws an error (unless used for modifiable style object):
Okay, thanks for the test, I guess there's more at play here indeed.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72591
> Yeah, I forgot to validate it in my branch. In BuiltinFuncCall, something like this would work:
But then you assume that all objects implementing the interface will use the same struct layout for that. I can't think of an example inside MSHTML where it's currently not the case, but that's not generally a promise that QI gives.
> Alternatively, we can pass around `func_info_t` instead of dispid and tid, but still look it up like in your branch.
So what's the point of it then? Avoiding one argument in a call?
> I mean the test will simply start failing if it picks `IHTMLCSSStyleDeclaration` because the assignment does not throw exception anymore (when it should be!).
I think that you misinterprets your observations. Here is a test proving that currentStyle should have a setter that throws an error (unless used for modifiable style object):
```
sync_test("currentStyle", function() {
if (document.documentMode < 9)
return;
var s = document.body.currentStyle;
var d = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(Object.getPrototypeOf(s)), "zoom");
ok(typeof(d.get) === "function", "d.get = " + d.get);
ok(typeof(d.set) === "function", "d.set = " + d.set);
d.set.call(document.body.style, "1");
try {
d.set.call(s, "1");
ok(false, "exception expected");
}catch(e) { trace("MSG: " + e.message); }
});
```
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72588
This MR adds support for creating file mapping objects backed by large pages on Linux, by making the following changes:
## wineserver
* On Linux, `create_temp_file` will first attempt to use memfds as the backing fd. If it fails, it'll return to the current codepath, creating a temporary file in either the server or config directory.
* The created memfd will be sealed against writes, if the caller requesting the appropriate page protection flags.
* This removes the requirement that FDs be only created on filesystems/directories that aren't `noexec`.
* In the server method `create_mapping` , if large pages have been requested by the caller, hold that the calling thread's token holds `SeLockMemoryPrivilege` .
* Additionally, add `SeLockMemoryPrivilege` to the list of privileges enabled for the Administrator.
## `ntdll`
* Add `virtual_get_min_large_page_size` and its exported wrapper `wine_unix_get_min_large_page_size`.
* On Linux, the minimum page size is determined by going through `/sys/kernel/mm/hugepages`. If hugepage support was not detected, `STATUS_NOT_SUPPORTED` is returned instead. On other platforms, the older hard-coded value of 2\*1024\*1024 is returned instead.
* `NtCreateSection` will validate certain parameters if large pages are requested. Specifically, it will return STATUS_INVALID_PARAMETER if the requested mapping is not anonymous/unnamed, or the size is not a multiple of the minimum supported page size.
## `kernelbase`
* `GetLargePageMinimum` will use `wine_unix_get_min_large_page_size`.
## `kernel32/tests`
* Add new test test_large_page_file_mapping, which validates privilege enforcements and parameter validation while creating large pages backed file mapping obejcts. The tests are skipped if `GetLargePageMinimum` returns 0.
--
v13: kernel32: Add tests for large page mapping support.
ntdll: Validate parameters to NtCreateSection if large pages are requested.
server: Use memfd to back anonymous mappings on Linux.
kernelbase: Implement GetLargePageMinimum by returning the value of LargePageMinimum in _KUSER_SHARED_DATA.
server: Set LargePageMinimum in _KUSER_SHARED_DATA on Linux.
server: Require SeLockMemoryPrivilege to create large page mappings.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5769
On Wed Jun 5 21:57:54 2024 +0000, Jacek Caban wrote:
> > But it was still ambiguous as I explained.
> If you mean the "zoom" example, I'd appreciate for some details. That
> one should already use `IHTMLCSSStyleDeclaration` interface, not
> `IHTMLStyle*` ones.
> > But since we pass the mshtml-internal tid (called iid in args) which
> is opaque to jscript, I don't really see why this is better than passing
> the `func_info_t` directly? Well IMO passing the pair is already more
> complex and requires more lookups, more args, etc, but that's just me.
> `get_dispex_from_this` is one thing, going through the interface take
> cares of that. For that part, you could just replace the argument in my branch.
> But we should also somehow make sure that the hook is safe to call. By
> binding function info to the exact object instance, we guarantee that
> it's safe to call `impl_from_*` from hooks. If you accept any function
> info as an input, you need to somehow validate that it's safe to call
> it. Something like moving QI earlier would only partially mitigate that.
> I'm open to other approaches, but unless I'm missing something, there is
> no such validation in your current branch.
> It seems to me that things like 1ffeaa446613 come from some variation of
> solution to that problem. Maybe it was needed in earlier version, but it
> shouldn't be needed with "this" being resolved on jscript side. It's no
> longer arbitrary when it's used for a `IJSDispatchHost` call. I would
> suggest to try removing it.
Yeah, I forgot to validate it in my branch. In BuiltinFuncCall,
something like this would work:
```c
if(func->hook) {
hres =
IWineJSDispatchHost_QueryInterface(&This->IWineJSDispatchHost_iface,
tid_ids[func->tid], (void**)&unk);
if(FAILED(hres))
return hres;
IUnknown_Release(unk);
hres = func->hook(This, setter ? DISPATCH_PROPERTYPUT :
DISPATCH_PROPERTYGET, dp, ret, ei, caller);
if(hres != S_FALSE)
return hres;
}
```
Alternatively, we can pass around `func_info_t` instead of dispid and
tid, but still look it up like in your branch. We can obtain the dispid
and tid from `func_info_t` so that's all that's needed. I'm not sure if
that's an improvement over my approach, but I think it's definitely a
simplification over yours. What do you think?
BTW for `zoom`, the assignment should fail as it does on native (as the
test does), are you sure it's supposed to be using
`IHTMLCSSStyleDeclaration` which doesn't fail? I mean the test will
simply start failing if it picks `IHTMLCSSStyleDeclaration` because the
assignment does not throw exception anymore (when it should be!).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72586
This MR adds support for creating file mapping objects backed by large pages on Linux, by making the following changes:
## wineserver
* On Linux, `create_temp_file` will first attempt to use memfds as the backing fd. If it fails, it'll return to the current codepath, creating a temporary file in either the server or config directory.
* The created memfd will be sealed against writes, if the caller requesting the appropriate page protection flags.
* This removes the requirement that FDs be only created on filesystems/directories that aren't `noexec`.
* In the server method `create_mapping` , if large pages have been requested by the caller, hold that the calling thread's token holds `SeLockMemoryPrivilege` .
* Additionally, add `SeLockMemoryPrivilege` to the list of privileges enabled for the Administrator.
## `ntdll`
* Add `virtual_get_min_large_page_size` and its exported wrapper `wine_unix_get_min_large_page_size`.
* On Linux, the minimum page size is determined by going through `/sys/kernel/mm/hugepages`. If hugepage support was not detected, `STATUS_NOT_SUPPORTED` is returned instead. On other platforms, the older hard-coded value of 2\*1024\*1024 is returned instead.
* `NtCreateSection` will validate certain parameters if large pages are requested. Specifically, it will return STATUS_INVALID_PARAMETER if the requested mapping is not anonymous/unnamed, or the size is not a multiple of the minimum supported page size.
## `kernelbase`
* `GetLargePageMinimum` will use `wine_unix_get_min_large_page_size`.
## `kernel32/tests`
* Add new test test_large_page_file_mapping, which validates privilege enforcements and parameter validation while creating large pages backed file mapping obejcts. The tests are skipped if `GetLargePageMinimum` returns 0.
--
v12: kernel32: Add tests for large page mapping support.
ntdll: Validate parameters to NtCreateSection if large pages are requested.
server: Use memfd to back anonymous mappings on Linux.
kernelbase: Implement GetLargePageMinimum by returning the value of LargePageMinimum in _KUSER_SHARED_DATA.
server: Set LargePageMinimum in _KUSER_SHARED_DATA on Linux.
server: Require SeLockMemoryPrivilege to create large page mappings.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5769
I have a .Net application that during handling of WM_NCPAINT for one of its windows
genarates exception "Arithmetic operation resulted in an overflow.". This happens
only in a 64-bit prefix and only for WPARAM that contains an HRGN handle with value
over than 0x7fffffff.
The application does the following:
public static IntPtr GetNativeDC(Message m, IntPtr handle)
{
if (m.Msg != 0x85) // WM_NCPAINT
{
return NUser32.GetWindowDC(handle);
}
int num = 0x200013; // DCX_VALIDATE | DCX_CLIPSIBLINGS | DCX_CACHE | DCX_WINDOW
IntPtr intPtr = IntPtr.Zero;
IntPtr wParam = m.WParam;
if (wParam.ToInt32() != 1)
{
num |= 0x80; // DCX_INTERSECTRGN
intPtr = CreateRectRgn(0, 0, 1, 1);
CombineRgn(intPtr, wParam, IntPtr.Zero, 5); // RGN_COPY
}
return NUser32.GetDCEx(handle, intPtr, num);
}
The exception is generated by wParam.ToInt32(). MSDN description for ToInt32()
https://learn.microsoft.com/en-us/dotnet/api/system.intptr.toint32?view=net…
states
Exceptions
OverflowException
In a 64-bit process, the value of this instance is too large or too small
to represent as a 32-bit signed integer.
MSDN also has a reference to ToInt32() source:
https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5…
public int ToInt32()
{
#if TARGET_64BIT
return checked((int)_value);
#else
return (int)_value;
#endif
}
It's not clear why a sign extension may lead to an overflow exception.
This patch fixes the problem, and provides a test case that confirms that
entry.Generation field of a GDI32 object is limited by 127 on a 64-bit
platform, while 32-bit Windows doesn't have such a limitation.
--
v2: win32u: Limit GDI object generation by 128.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5777
[test_WaitForInputIdle.tar.bz2](/uploads/a20c5b119741cd7b0b4813488854a992/test_WaitForInputIdle.tar.bz2)
Attached test replicates the problem. Basically the application does
process = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId());
ret = WaitForInputIdle(process, 0x7fffffff);
assert(ret == 0);
With current wine.git WaitForInputIdle() returns WAIT_FAILED because the server call
get_process_idle_event() refuses to return idle_event for the current process. If that
limitation is removed then wineserver crashes, and other parts of the patch fix the crash.
--
v2: server: Remove limitation for waiting on idle_event of the current process.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5789
Instead of accessing the surface with macdrv_get_surface_display_image.
--
v2: winemac: Remove now unnecessary cocoa window surface pointer.
winemac: Push window surface image updates to the main thread.
winemac: Create window surface CGImageRef on surface flush.
winemac: Create a provider for the surface and a HBITMAP wrapping it.
winemac: Remove unused macdrv_get_surface_display_image copy_data parameter.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5798
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions:
1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves).
Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
--
v5: win32u: Implement keyboard auto-repeat using new server request.
server: Implement key auto-repeat request.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions:
1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves).
Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
--
v6: win32u: Implement keyboard auto-repeat using new server request.
server: Implement key auto-repeat request.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
--
v3: win32u: Pass the rect DPI to NtUserIsWindowRectFullScreen.
win32u: Introduce a new get_monitor_rect helper.
win32u: Pass desired DPI to NtUserGet(Client|Window)Rect.
win32u: Introduce NtUserAdjustWindowRect call for AdjustWindowRect*.
win32u: Introduce new helpers to convert server rectangle_t.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5776
Jacek Caban (@jacek) commented about dlls/wininet/http.c:
>
> if (maxlen == -1) maxlen = sizeof(req->read_buf);
>
> + NETCON_set_timeout(req->netconn, FALSE, req->hdr.receive_timeout);
Setting the timeout on the socket is quite expensive (requires a wineserver call). Maybe we could store the last set timeout separate from configure timeout and update socket timeout only when they don't match?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3518#note_72541
It looks better, thanks. In addition to minor comments, please split it. Additional `NETCON_set_timeout` fix is a separate fix from setting/getting options. Maybe also separate `NETCON_set_timeout` for send and receive.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3518#note_72542
Jacek Caban (@jacek) commented about dlls/wininet/internet.c:
> + if (!lpwhh)
> + {
> + SetLastError(ERROR_INTERNET_INCORRECT_HANDLE_TYPE);
> + return FALSE;
> + }
> + if (!lpBuffer || dwBufferLength != sizeof(ULONG))
> + {
> + SetLastError(ERROR_INVALID_PARAMETER);
> + ret = FALSE;
> + }
> + else
> + {
> + lpwhh->connect_timeout = *(ULONG *)lpBuffer;
> + }
> + break;
> + }
Do we still need it? I think that with implementation in both `set_global_option` and `INET_SetOption`, we could simply remove those cases from `InternetSetOptionW` fallback.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3518#note_72540
> But it was still ambiguous as I explained.
If you mean the "zoom" example, I'd appreciate for some details. That one should already use `IHTMLCSSStyleDeclaration` interface, not `IHTMLStyle*` ones.
> But since we pass the mshtml-internal tid (called iid in args) which is opaque to jscript, I don't really see why this is better than passing the `func_info_t` directly? Well IMO passing the pair is already more complex and requires more lookups, more args, etc, but that's just me.
`get_dispex_from_this` is one thing, going through the interface take cares of that. For that part, you could just replace the argument in my branch.
But we should also somehow make sure that the hook is safe to call. By binding function info to the exact object instance, we guarantee that it's safe to call `impl_from_*` from hooks. If you accept any function info as an input, you need to somehow validate that it's safe to call it. Something like moving QI earlier would only partially mitigate that. I'm open to other approaches, but unless I'm missing something, there is no such validation in your current branch.
It seems to me that things like 1ffeaa446613 come from some variation of solution to that problem. Maybe it was needed in earlier version, but it shouldn't be needed with "this" being resolved on jscript side. It's no longer arbitrary when it's used for a `IJSDispatchHost` call. I would suggest to try removing it.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72533
On Wed Jun 5 18:25:43 2024 +0000, Jacek Caban wrote:
> > Well yes, of course they check for the object type compatibility, but
> in the end they should still preserve their "function identity" instead
> of being looked up from the object everytime, even if it's completely
> detached from it.
> I don't know what you mean by that, a pair like (dispid, iid) uniquely
> defines a function identity. There is nothing special about dispid
> except that we already need its lookup implemented, so that seems convenient.
> See https://gitlab.winehq.org/jacek/wine/-/commits/func-call for a draft
> of what I mean. It implements `call` only for legacy functions, but I
> think it would scale to proper prototypes. It implements
> bc02a4fe3be595448, but doesn't need unpleasant things like
> `get_dispex_from_this` and uses the new interface instead (so committing
> something like that would also be a good opportunity to upstream
> "boring" parts of 81800d9dfa59f4fd8).
> Extending it with accessors support should be straightforward. For
> prototypes, we could just have entries for those functions both in the
> instance and prototype objects. In instances, they would be not searched
> when doing lookups by name, so the only way to use it would be through
> `IJSDispatchHost`. For prototypes, they would be looked up, but an
> attempt to call it would fail because prototypes don't actually
> implement the required interface (so the result of the lookup would be
> useful only for instances). It should also allow skipping 1ffeaa446613
> with all its consequences. What do you think?
Well yes, a pair does work, but I wasn't passing the IID, only the
DISPID after making sure the object implements the required IID, since I
was delegating to the standard InvokeEx (well, almost), which only has
DISPID. But it was still ambiguous as I explained.
But since we pass the mshtml-internal tid (called iid in args) which is
opaque to jscript, I don't really see why this is better than passing
the `func_info_t` directly? Well IMO passing the pair is already more
complex and requires more lookups, more args, etc, but that's just me.
I haven't analyzed the rest yet, I'll look tomorrow as I have to go now,
but `get_dispex_from_this` is primarily useful for jscript proxies since
they can have arbitrary "this" passed. I was using it here (for legacy
function.call) mostly to introduce it earlier and have less to add on
the proxy patch which was already big enough as it is, but it's not its
main purpose. It was just a "split" attempt.
Are you sure you can get rid of it with your method?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72511
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions:
1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves).
Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
--
v4: win32u: Implement keyboard auto-repeat using new server request.
server: Implement key auto-repeat request.
server: Check message target process for raw input device flags.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
> Well yes, of course they check for the object type compatibility, but in the end they should still preserve their "function identity" instead of being looked up from the object everytime, even if it's completely detached from it.
I don't know what you mean by that, a pair like (dispid, iid) uniquely defines a function identity. There is nothing special about dispid except that we already need its lookup implemented, so that seems convenient.
See https://gitlab.winehq.org/jacek/wine/-/commits/func-call for a draft of what I mean. It implements `call` only for legacy functions, but I think it would scale to proper prototypes. It implements bc02a4fe3be595448, but doesn't need unpleasant things like `get_dispex_from_this` and uses the new interface instead (so committing something like that would also be a good opportunity to upstream "boring" parts of 81800d9dfa59f4fd8).
Extending it with accessors support should be straightforward. For prototypes, we could just have entries for those functions both in the instance and prototype objects. In instances, they would be not searched when doing lookups by name, so the only way to use it would be through `IJSDispatchHost`. For prototypes, they would be looked up, but an attempt to call it would fail because prototypes don't actually implement the required interface (so the result of the lookup would be useful only for instances). It should also allow skipping 1ffeaa446613 with all its consequences. What do you think?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72509
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698
--
v6: quartz: Allow concurrent calls to AVI decoder qc_Notify and Receive.
msvfw32/tests: Test that Cinepak rejects unsupported output types.
iccvid: Reject unsupported output types.
quartz/tests: Add Cinepak test to avi splitter.
winegstreamer: Use end of previous frame if the current frame doesn't have a timestamp.
winegstreamer: Implement AM_MEDIA_TYPE to wg_format converter for Cinepak video.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5744
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions:
1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves).
Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
--
v3: win32u: Implement keyboard auto-repeat using new server request.
server: Implement key auto-repeat request.
server: Check message target process for raw keyboard device flags.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
On Wed Jun 5 13:11:29 2024 +0000, cqwrteur wrote:
> Of course, there is. The problem is that ucrt can be statically linked
> from Microsoft UCRT static ucrt can be put back to FILE* in dynamic
> linking. And fast_io library is using that.
gitlab is not showing the context very well. I was asking about `stdout == stdin+1` test. Does the library depend on that?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72456
On Wed Jun 5 13:02:30 2024 +0000, cqwrteur wrote:
> It is not. The problem is that ucrt can be statically linked from
> Microsoft UCRT static ucrt can be put back to FILE* in dynamic linking.
> And fast_io library is exploiting that to gain huge performance.
The `ok(ptrold + cntold == file->_ptr + file->_cnt, ...);` test is redundant. You're testing `ptrold` and `cntold` values in earlier tests.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72454
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions:
1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves).
Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
--
v2: win32u: Remove auto-repeat functionality.
server: Implement key auto-repeat.
server: Check message target process for raw keyboard device flags.
user32: Add tests for cross-process raw keyboard events.
server: Pass desktop to get_first_global_hook.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
--
v2: wineps.drv: Add partial support for changing page size.
wineps.drv: Write PageBoundingBox for every page.
wineps.drv: Take all pages into account when computing bounding box.
wineps.drv: Write page orientation hint for every page.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5791
[test_WaitForInputIdle.tar.bz2](/uploads/a20c5b119741cd7b0b4813488854a992/test_WaitForInputIdle.tar.bz2)
Attached test replicates the problem. Basically the application does
process = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId());
ret = WaitForInputIdle(process, 0x7fffffff);
assert(ret == 0);
With current wine.git WaitForInputIdle() returns WAIT_FAILED because the server call
get_process_idle_event() refuses to return idle_event for the current process. If that
limitation is removed then wineserver crashes, and other parts of the patch fix the crash.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5789
This adds some wine-only messages, but most of those are cases that follow an existing EVENT_SYSTEM_FOREGROUND wine-only message, so likely correctly reflecting a thing Wine is already known to do wrong.
The exception is WmRestore_3. https://testbot.winehq.org/JobDetails.pl?Key=146025 suggests that's also a case where focuses the window and shouldn't.
--
v2: win32u: Send EVENT_OBJECT_FOCUS in more cases.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5779
Two main changes:
- VM configuration in build.yml for the executor.
- The build-mac script is now architecture-agnostic.
--
v3: gitlab: Update configuration for the new Mac runner.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5749
The Windows failures here are in a different test unit. They don't look like they should be intermittent, but they don't seem to happen in winetest runs. I don't know what's different about this environment...
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5793#note_72413
This reduce the chance of buffer allocating issue for Kirikiri2 games(eg. Super Naughty Maid) when the game is trying to play videos.
--
v2: qasf/dmowrapper: Sync Stop() and Receive() for dmo wrapper filter.
qasf/dmowrapper: Return VFW_E_WRONG_STATE in dmo_wrapper_sink_Receive.
qasf/dmowrapper: Return failure in dmo_wrapper_sink_Receive if samples allocating fails.
qasf/dmowrapper: Introduce release_output_samples.
qasf/dmowrapper: Introduce get_output_samples.
qasf/tests: Add more tests for dmo_wrapper_sink_Receive.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
--
v2: winegstreamer: Request the new transform output format explicitly.
winegstreamer: Get rid of the wg_transform internal output_format.
winegstreamer: Only report format changes when frontend supports it.
winegstreamer: Rename allow_size_change to allow_format_change.
winegstreamer: Enforce default stride presence in the video processor.
winegstreamer: Enforce default stride value in the video decoder.
winegstreamer: Allow to clear video decoder input/output types.
https://gitlab.winehq.org/wine/wine/-/merge_requests/5732
On Tue Jun 4 22:57:24 2024 +0000, cqwrteur wrote:
> It was always different in layout. This is a bug for years. UCRT does
> not have the same layout of FILE compared to msvcrt. However wine just
> assumes they are the same, which is wrong. MSVC allows you to pass FILE*
> from a statically linked ucrt to a dynamic linked ucrt provided by the
> operating system and that is of course causing abi break.
https://github.com/cppfastio/fast_io/blob/next/include/fast_io_legacy_impl/…
I have changed my code to make it clearer. msvcrt and ucrt always use different layouts. This should be clearer for you.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72402
On Sun Jun 2 22:51:59 2024 +0000, cqwrteur wrote:
> changed this line in [version 21 of the diff](/wine/wine/-/merge_requests/5752/diffs?diff_id=116242&start_sha=0ed3968e5c80f1068a89193deb4b3309a69d8e27#1a28d67ae45bfb1e18198f92ca618b484c4e5a63_3171_3164)
It is not. The problem is that ucrt can be statically linked from Microsoft UCRT static ucrt can be put back to FILE* in dynamic linking. And fast_io library is exploiting that to gain huge performance.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72395
On Sun Jun 2 22:59:20 2024 +0000, cqwrteur wrote:
> changed this line in [version 22 of the diff](/wine/wine/-/merge_requests/5752/diffs?diff_id=116244&start_sha=f9ce09a15be1de01f4b8f3f2c25cf9c722a4b2d8#1a28d67ae45bfb1e18198f92ca618b484c4e5a63_3144_3149)
It was always different in layout. This is a bug for years. UCRT does not have the same layout of FILE compared to msvcrt. However wine just assumes they are the same, which is wrong. MSVC allows you to pass FILE* from a statically linked ucrt to a dynamic linked ucrt provided by the operating system and that is of course causing abi break.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72396
On Sun Jun 2 22:51:59 2024 +0000, cqwrteur wrote:
> changed this line in [version 21 of the diff](/wine/wine/-/merge_requests/5752/diffs?diff_id=116242&start_sha=0ed3968e5c80f1068a89193deb4b3309a69d8e27#1a28d67ae45bfb1e18198f92ca618b484c4e5a63_3173_3164)
It should be fixed. Please review it again
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72397
On Tue Jun 4 22:57:24 2024 +0000, Piotr Caban wrote:
> Please move the test to `dlls/ucrtbase/tests/file.c`. Also please add
> tests in separate patch.
What do you mean by a separate patch?
Test Moved.
Is it ok for you now?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72398
On Sun Jun 2 22:51:57 2024 +0000, cqwrteur wrote:
> changed this line in [version 21 of the diff](/wine/wine/-/merge_requests/5752/diffs?diff_id=116242&start_sha=0ed3968e5c80f1068a89193deb4b3309a69d8e27#1a28d67ae45bfb1e18198f92ca618b484c4e5a63_3153_3158)
Of course, there is. The problem is that ucrt can be statically linked from Microsoft UCRT static ucrt can be put back to FILE* in dynamic linking. And fast_io library is using that.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5752#note_72394