return values of xmlutil functions are not being consistent... #4899

Closed
wixbot opened this Issue Sep 21, 2015 · 5 comments

Comments

Projects
None yet
2 participants
Collaborator

wixbot commented Sep 21, 2015

I am trying to turn off ShowFilesInUse, however it looks like the return values of the xmlutil functions aren't being consistent.
The XmlGetAttributeEx returns E_NOTFOUND, but XmlGetAttribute returns S_FALSE if the attribute isn't found (and as such so does XmlGetAttributeNumber).

From Jacob Hoover:

"Since no is the default value, it would make sense for it to not be written to the manifest.

https://msdn.microsoft.com/en-us/library/ms767592(v=vs.85).aspx

If that documentation is correct...

Return Values
S_OK
The value returned if successful.
S_FALSE
The value when returning Null.
E_INVALIDARG
The value returned if the namedItem parameter is Null.

That leads me to believe that XmlGetAttributeNumber is returning S_FALSE not E_NOTFOUND. In which case, it's turning on the files in use dialog because of the previous value being read in from the last option it found.

I sure hope I am reading the docs wrong, or this is a rather large and long standing bug.
"
and the solution:

"Steven,
I think it's safe to say this is a bug, where the return values of the xmlutil functions aren't consistent. The XmlGetAttributeEx returns E_NOTFOUND, but XmlGetAttribute returns S_FALSE if the attribute isn't found (and as such so does XmlGetAttributeNumber).

This bug has existed since WIX 3.8, but if you would log it, we can bring it up for discussion in tomorrow's meeting.

The short term fix, if you can build Wix, is to change E_NOTFOUND to S_FALSE for anything that calls XmlGetAttribute inside of WixStdBA.

if (E_NOTFOUND == hr) <--- This compare.
{
hr = S_OK;
}
else if (SUCCEEDED(hr))
{
...
}
"

i.e. short term solution for me:
if (E_NOTFOUND == hr) <--- This compare.
{
hr = S_OK;
}
else if (SUCCEEDED(hr))
{
...
}

To:

if (S_FALSE == hr) <--- This compare.
{
hr = S_OK;
}
else if (SUCCEEDED(hr))
{
...
}

ps actually the whole text of this bug was from Jacob :) (thank you sir)

Originally opened by sogilvie

Collaborator

wixbot commented Sep 22, 2015

There are 2 consumers of XmlGetNamedItem:

XmlGetAttribute:

hr = XmlGetNamedItem(pixnnmAttributes, bstrAttribute, &pixnAttribute);
if (S_FALSE == hr)
{
    // hr = E_FAIL;
    ExitFunction();
}

and XmlGetAttributeEx

hr = XmlGetNamedItem(pixnnmAttributes, bstrAttribute, &pixnAttribute);
if (S_FALSE == hr)
{
    ExitFunction1(hr = E_NOTFOUND);
}

Originally posted by jchoover

Collaborator

wixbot commented Sep 22, 2015

Looks like it's always returned S_FALSE for the XmlGetAttribute (and the methods that consume it), however I do have concerns with the following calls (note the WixStBA line numbers are wrong, as I am doing this check on my branch with local mods to WixStdBA):

wix3\src\burn\engine\container.cpp(91):        hr = XmlGetAttributeNumber(pixnNode, L"AttachedIndex", &pContainer->dwAttachedIndex);
wix3\src\burn\engine\msiengine.cpp(83):    hr = XmlGetAttributeNumber(pixnMsiPackage, L"Language", &pPackage->Msi.dwLanguage);
wix3\src\burn\engine\msiengine.cpp(1464):            hr = XmlGetAttributeNumber(pixnNode, L"Id", &pRelatedMsi->rgdwLanguages[i]);
wix3\src\burn\engine\package.cpp(151):        hr = XmlGetAttributeLargeNumber(pixnNode, L"Size", &pPackage->qwSize);
wix3\src\burn\engine\package.cpp(155):        hr = XmlGetAttributeLargeNumber(pixnNode, L"InstallSize", &pPackage->qwInstallSize);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1796):        hr = XmlGetAttributeNumber(pNode, L"SuppressOptionsUI", &dwBool);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1808):        hr = XmlGetAttributeNumber(pNode, L"SuppressDowngradeFailure", &dwBool);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1820):        hr = XmlGetAttributeNumber(pNode, L"SuppressRepair", &dwBool);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1831):        hr = XmlGetAttributeNumber(pNode, L"ShowVersion", &dwBool);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1842):        hr = XmlGetAttributeNumber(pNode, L"SupportCacheOnly", &dwBool);
wix3\src\ext\BalExtension\wixstdba\WixStandardBootstrapperApplication.cpp(1853):        hr = XmlGetAttributeNumber(pNode, L"ShowFilesInUse", &dwBool);

Originally posted by jchoover

Collaborator

wixbot commented Sep 22, 2015

Area changed from burn to sdk
AssignedTo set to jchoover
Release changed from v3.10 to v4.x

Collaborator

wixbot commented Sep 29, 2015

Release changed from v4.x to v3.10

Collaborator

wixbot commented Oct 30, 2015

Originally changed by barnson
Resolution set to fixed
Status changed from Open to Resolved

@wixbot wixbot added bug sdk labels Dec 20, 2015

jchoover was assigned by wixbot Dec 20, 2015

wixbot added this to the v3.10 milestone Dec 20, 2015

wixbot closed this Dec 20, 2015

@robmen robmen added a commit to wixtoolset/wix4 that referenced this issue Mar 22, 2016

@jchoover @robmen jchoover + robmen WIXBUG:4899 - Modified WixStdBA handling of XmlGetAttribute to handle…
… S_FALSE.

Addresses wixtoolset/issues#4899.
2f5b441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment