On 05/04/2013 05:13 PM, Max TenEyck Woodbury wrote:
Having wine throw an exception where it did not do so before is another
kind of problem indicator. One that hardly needs a special conformance
test.
Hmm. As I suspected, that is a single point pass only test. It does
not explore any of the possible fail conditions. Thus, it is also
definitely a possibility that Windows makes no such sanity check.
I suggested the conformance test for removing the sanity check, not for
the crash. We need to know for sure whether Windows makes no such sanity
check before we act on that possibility.
Almost certainly. Without this patch, the 'make test' suit throws a
divide by zero exception and brings up a dialog box. With this patch,
it does not. I believe that is sufficient to convict the unpatched
code of having something wrong with it.
The flaw in your logic is that you're assuming the code which generates
the exception is necessarily the code that's written incorrectly. By the
same logic, you could patch out the crash handler and say "Look, no more
crash dialog! It's fixed!"
I agree that the unpatched code is definitely dividing by zero in this
case; but according to Dmitry, tmHeight is never supposed to be 0. If
he's correct, it means that this code is correct (given the assumption),
but something else is bugged (thereby violating that "contract of
behavior") and that's what should be fixed instead.
As Dmitry has said,
tmHeight should never be 0, so it's probably valid to assume
tmHeight!=0.
But that assumption can be checked. Currently there is no such check.
'Should' in this context is a very bad word and has no place at the
foundation of an argument about what actually happens.
I agree that this assumption can/should be checked, but the appropriate
place to do that is in unit tests. This also verifies that the
assumption holds on Windows as well, ensuring that Wine's behavior
doesn't diverge from that of Windows.
And yeah, sorry for using "should" - it is kind of a weak word, in
retrospect... I think "must" would have been a better choice, but I'm
not certain enough for a word so strong: We need to see what tmHeight
does on Windows (that means tests!) before we can argue it one way or
another. :)
The bug may instead be in allowing the font to load with
tmHeight=0, and this is merely the first crash that the problem causes
for you. But what about apps that divide by tmHeight under the same
assumption? We can't change those, so it's better to fix tmHeight.
If the APP does the division, that is the APP's problem. Wine, on the
other hand should not throw an exception, and it did NOT do so until
recently. Whoever wrote the new code (Dmitry?) should have put in the
check or made the test work without doing a division. The fact that it
fails is the problem being addressed.
If the app, which was made for Windows, works on Windows, but not on
Wine, then it is our problem, not the app's. The crash that you're
seeing is a clear indicator of *something* being wrong, but we don't yet
know specifically what.
I have no objection to someone writing an alternative patch and backing
this one out when that patch goes in, but until then, this patch, or
something like it, needs to be applied. With wine throwing the
exception, some Apps are going to fail that would not fail otherwise.
That is definitely a 'Bad Thing' and should be fixed ASAP (like right
now)!
Crashes are definitely a Bad Thing(TM), but this patch may not fix all
of them (e.g. with apps that assume tmHeight!=0) without actually fixing
the underlying problem. In fact, this patch itself could be a Bad
Thing(TM) -- if something is wrong with tmHeight calculation/scaling,
the patch might only mask the real issue, which would make a proper fix
take even longer.
-----
I would like to reiterate: I simply don't know what tmHeight is supposed
to do. According to Dmitry, the real bug is having tmHeight=0. If
Windows allows tmHeight=0 under some circumstance, then your patch is
the right thing to do, because we must prevent a division by zero in
that case. If Windows absolutely disallows tmHeight=0, then the bug is
actually that Wine allows tmHeight=0, and so we should be focusing on
that instead.
Hope this helps,
Sam
