[Win] Fix the wrong return value checking of ScriptItemize in UniscribeController::itemizeShapeAndPlace UniscribeController::itemizeShapeAndPlace has the following code. > while (rc = ::ScriptItemize(cp, length, m_items.size() - 1, &m_control, &m_state, m_items.data(), &numItems) == E_OUTOFMEMORY) { > m_items.resize(m_items.size() * 2); > resetControlAndState(); > } > if (FAILED(rc)) { > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > return; > } > rc is aligned 0 or 1, and checked by FAILED(rc). '==' has higher operator precedence than '='. It is interpreted as: > rc = (::ScriptItemize(...) == E_OUTOFMEMORY) https://en.cppreference.com/w/cpp/language/operator_precedence This code has been introduced in Bug 137526.
FAILED macro | Microsoft Docs https://docs.microsoft.com/en-us/windows/desktop/api/winerror/nf-winerror-failed > #define FAILED(hr) (((HRESULT)(hr)) < 0) FAILED(rc) is always false. ScriptItemize function | Microsoft Docs https://docs.microsoft.com/en-us/windows/desktop/api/usp10/nf-usp10-scriptitemize > The function returns E_INVALIDARG if pwcInChars is set to NULL, cInChars is 0, pItems is set to NULL, or cMaxItems < 2. If I understand the code correctly, ScriptItemize never returns E_INVALIDARG in above code. There is no way to test.
Created attachment 357542 [details] Patch
Comment on attachment 357542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357542&action=review The change seems ok. The log seems problematic. > Source/WebCore/ChangeLog:22 > + Fortunately, as far as I understand the code, there is not an > + actual problem, and no way to test the coding error. If this is true we should just remove all this code. > Source/WebCore/ChangeLog:24 > + No new tests, no behavior changes. There is a behavior change.
Do you mean the ScriptItemize can return E_INVALIDARG in the code? The code has been introduced to resolve static analyzer warnings in Bug 137526. I guess the resolve static analyzer was not smart enough to know ScriptItemize never return E_INVALIDARG in the condition. I guess the resolve static analyzer simply reported E_INVALIDARG is not handled.
s/the resolve static analyzer/the static analyzer/
I think the following condition never be true because ScriptItemize never return E_INVALIDARG in the code. > if (FAILED(rc)) { > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > return; > } But, if I remove the above code, the varialbe 'rc' become unsued. Then, I need to remove all 'rc'. the static analyzer ends up reporting the warning again.
(In reply to Fujii Hironori from comment #6) > I think the following condition never be true because ScriptItemize never > return E_INVALIDARG in the code. > > > if (FAILED(rc)) { > > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > > return; > > } > > But, if I remove the above code, the varialbe 'rc' become unsued. Then, I > need to remove all 'rc'. > the static analyzer ends up reporting the warning again. I'm able to remove the condition by using ASSERT_UNUSED macro. > ASSERT_UNUSED(rc, SUCCEEDED(rc)); // ScriptItemize never returns E_INVALIDARG. The benefit of this approach is there would be no behavior change even if ScriptItemize returns E_INVALIDARG in Release build.
Created attachment 358103 [details] Patch
Created attachment 358104 [details] Patch
Review, please.
Comment on attachment 358104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358104&action=review > Source/WebCore/platform/graphics/win/UniscribeController.cpp:203 > + ASSERT_UNUSED(rc, SUCCEEDED(rc)); // ScriptItemize must not return E_INVALIDARG in this code. I'm afraid I don't understand the benefit of this change. I know that you found MSDN documentation that claims that ScriptItemize returns E_INVALIDARG in some cases, and is silent about other error conditions. But I don't think that's sufficient evidence to assume that ScriptItemize will never return some other error value (or might be modified in the future to do so). Why not leave this error check in place?
Thank you for the feedback. Do you prefer the first patch? I have no preference to both approaches. I removed the error checking based on Alex's feedback (Comment 3).
I removed the code in r254323. Closed.