Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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

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

wixbot opened this issue Sep 21, 2015 · 5 comments
Assignees
Milestone

Comments

@wixbot
Copy link

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

@wixbot
Copy link
Author

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

@wixbot
Copy link
Author

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

@wixbot
Copy link
Author

wixbot commented Sep 22, 2015

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

@wixbot
Copy link
Author

wixbot commented Sep 29, 2015

Release changed from v4.x to v3.10

@wixbot
Copy link
Author

wixbot commented Oct 30, 2015

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

@wixbot wixbot added this to the v3.10 milestone Dec 20, 2015
@wixbot wixbot closed this as completed Dec 20, 2015
rseanhall pushed a commit to rseanhall/wix4-archive that referenced this issue Mar 19, 2016
robmen pushed a commit to wixtoolset/wix4-archive that referenced this issue Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants