Peter Schlaile peter@schlaile.de writes:
Fix: X11DRV_SetDIBits() shouldn't silently change Bitmap protection, so we now restore the protection bits, that were present before instead of always changing to READONLY.
No, it needs to be read-only so that we can detect further changes.
Additional minor fix: bmi_has_alpha() tests for alpha channel != 0, but should test for alpha channel != 255.
No it shouldn't, 0 is correct.
Hi Alexandre,
On Mon, 31 Jan 2011, Alexandre Julliard wrote:
Peter Schlaile peter@schlaile.de writes:
Fix: X11DRV_SetDIBits() shouldn't silently change Bitmap protection, so we now restore the protection bits, that were present before instead of always changing to READONLY.
No, it needs to be read-only so that we can detect further changes.
so: what is your proposal to fix the problem?
At least, wine reacts very differently than windows here :) Read: it crashes the application I mentioned in my previous mail: http://www.phononet.de/downloads/support/downloads/Aktueller_PNClient/PNCLIE...
And: from a design perspective it sounds very strange that a fast track optimisation *silently* changes protection bits!
That wouldn't be a problem, if windows behaved exactly the same way. (which it most likely does not, otherwise PNClient wouldn't crash, right?).
And: my problem is, that I already spent three days in tracking this bug down, so, I'm not sure, if I find the time to write test code, that will demonstrate the difference between windows and wine.
Should we move to bug tracker then?
Additional minor fix: bmi_has_alpha() tests for alpha channel != 0, but should test for alpha channel != 255.
No it shouldn't, 0 is correct.
Are you sure? 0 means: everything is transparent, and that sounds like: we need an alpha channel, right?
Don't get me wrong, if windows behaves like that, it's ok for me, too, but that really sounds strange.
Cheers, Peter
---- Peter Schlaile
Peter Schlaile peter@schlaile.de writes:
At least, wine reacts very differently than windows here :) Read: it crashes the application I mentioned in my previous mail: http://www.phononet.de/downloads/support/downloads/Aktueller_PNClient/PNCLIE...
And: from a design perspective it sounds very strange that a fast track optimisation *silently* changes protection bits!
There's nothing silent about it, the protection bits have to match the DIB state.
That wouldn't be a problem, if windows behaved exactly the same way. (which it most likely does not, otherwise PNClient wouldn't crash, right?).
I wouldn't be surprised if there was a way to crash it on Windows too, that app is broken, it never gives the code a chance to handle the exception. This is not how exception handlers are supposed to work. Of course this specific issue would be fixed by a DIB engine, but there are other places where exceptions can happen internally, even on Windows.
Are you sure? 0 means: everything is transparent, and that sounds like: we need an alpha channel, right?
0 means there's no alpha channel, which is what we are checking here.
Hi Alexandre,
On Mon, 31 Jan 2011, Alexandre Julliard wrote:
And: from a design perspective it sounds very strange that a fast track optimisation *silently* changes protection bits!
There's nothing silent about it, the protection bits have to match the DIB state.
Hmm. In which way changes StretchDIBits() the DIB state? Or: why should be a DIB, that is created using CreateDIBSection() as READWRITE, silently change it's state into READONLY? (At least, msdn-docs don't tell me anything about that... Can you provide me with a link? )
But, let's say for a moment, that this is true and the right(tm) behaviour:
Then create_alpha_bitmap() (within user32/cursoricon.c) seems to be broken.
It does the following:
* create a DIBSection
alpha = CreateDIBSection( hdc, info, DIB_RGB_COLORS, &bits, NULL, 0)
* here we can savely expect bits to be writable (at least the msdn-documentation says so)
* in certain cases (if we are provided with a src_info-pointer), we do now:
StretchDIBits( hdc, 0, 0, bm.bmWidth, bm.bmHeight, 0, 0, src_info->bmiHeader.biWidth, src_info->bmiHeader.biHeight, color_bits, src_info, DIB_RGB_COLORS, SRCCOPY );
* after that, bits is *probably* non-writable (depending on fast path or not) (I still find that *really* strange. Again: it depends on certain parameters of this function, specifically: if in and out parameters do match at enough places, the DIB bitmap *changes* it's state to READONLY, in other cases it doesn't. If that isn't a big surprise, what is? :) ).
* and here...
/* pre-multiply by alpha */ for (i = 0, ptr = bits; i < bm.bmWidth * bm.bmHeight; i++, ptr += 4) { unsigned int alpha = ptr[3]; ptr[0] = ptr[0] * alpha / 255; ptr[1] = ptr[1] * alpha / 255; ptr[2] = ptr[2] * alpha / 255; }
* ... we write to the now readonly bits-array, which leads to a page fault.
This is everything within wine code, the application only called CreateIconFromResourceEx(), which then called create_alpha_bitmap() (so: no, I don't think, the app is broken :) It only triggered a seldomly used code path within wine. ).
Where is the page fault handler within wine and what should the page fault handler actually do in that situation?
If we can't expect the "bits"-pointer to be writable after a call to StretchDIBs(), create_alpha_bitmap() should be rewritten.
That wouldn't be a problem, if windows behaved exactly the same way. (which it most likely does not, otherwise PNClient wouldn't crash, right?).
I wouldn't be surprised if there was a way to crash it on Windows too, that app is broken, it never gives the code a chance to handle the exception. This is not how exception handlers are supposed to work.
Well, it doesn't crash on Windows and is actually rock solid there.
And: do you want to say, that there is a page fault handler within wine, that can handle *that* case above?
We either have to rewrite create_alpha_bitmap() in a way, that doesn't call StretchDIBits() in the fast path case and solves problems differently or: we should fix it at the source (which I still think my patch does, since it follows the rules of least surprise...).
In either way, I hope we can agree on the following:
a) the app is *not* broken (at least not regarding it's usage of CreateIconFromResourceEx() ) b) wine code *is* broken here c) we can work around the problem within create_alpha_bitmap() or d) make StretchDIBits() a function a lot less surprising to call...
Of course this specific issue would be fixed by a DIB engine, but there are other places where exceptions can happen internally, even on Windows.
At least, they seem to install the handler in such a clever way, that no problems seem to occur within Windows (several different versions). That said: I'd have prefered, if they didn't do that, since it made my debugging session a lot longer...
Are you sure? 0 means: everything is transparent, and that sounds like: we need an alpha channel, right?
0 means there's no alpha channel, which is what we are checking here.
Ouch! If that is really Windows behaviour, then it is really broken but bug-to-bug compatible :)
Cheers, Peter
---- Peter Schlaile
Peter Schlaile peter@schlaile.de writes:
And: do you want to say, that there is a page fault handler within wine, that can handle *that* case above?
Yes, that's how DIBs work in Wine.
In either way, I hope we can agree on the following:
a) the app is *not* broken (at least not regarding it's usage of CreateIconFromResourceEx() ) b) wine code *is* broken here c) we can work around the problem within create_alpha_bitmap() or d) make StretchDIBits() a function a lot less surprising to call...
There's nothing surprising here. The page protection is transparent to the app, and exceptions are handled by Wine. It's only because the app breaks exception handling that you get problems.
At least, they seem to install the handler in such a clever way, that no problems seem to occur within Windows (several different versions). That said: I'd have prefered, if they didn't do that, since it made my debugging session a lot longer...
They don't install it in a clever way, that's the whole problem. They simply blindly override any other installed handler. The only way to fix it apart from writing a DIB engine would be to add a special hack to make the DIB exception handler take priority over the app broken handler.
-
Alexandre Julliard -
Peter Schlaile