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

Jb/issue 1613 #1668

Merged
merged 2 commits into from
Jan 18, 2017
Merged

Jb/issue 1613 #1668

merged 2 commits into from
Jan 18, 2017

Conversation

jboich
Copy link
Contributor

@jboich jboich commented Jan 10, 2017

I haven't write unit tests so far: I'm not sure what data to use to mock symbols in different languages as input.
Using CFURLGetFileSystemRepresentation seems right for this issue


This change is Reviewable

@msftclas
Copy link

Hi @jboich, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;


char cPath[CFMaxPathSize];
if (CFURLGetFileSystemRepresentation(url, true, (unsigned char *)cPath, CFMaxPathSize)) {
success = _CFCreateDirectory(cPath);
Copy link
Member

@bbowman bbowman Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

   [](start = 0, length = 2)

Please make sure to run clang-format before submitting any code. In this case you have tabs that should be spaces. Coding conventions are a sticking point for a lot of people so we make it pretty easy to ensure the code is conformant (https://github.com/Microsoft/WinObjC/wiki/ClangFormat) for more info.

EDIT: Just realized that core foundation turns off clang-format because we don't want to make unecessary churn from the ref platform. Can you just manually change your tabs to spaces? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just manually change your tabs to spaces?

sure, will do

_CFCreateDirectory(CFStringGetCStringPtr(completePath, kCFStringEncodingUTF8)); // WINOBJC: ensure directory exists

char cPath[CFMaxPathSize];
if (CFURLGetFileSystemRepresentation(url, true, (unsigned char *)cPath, CFMaxPathSize)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure any and all changes to CoreFoundation are annotated with a // WINOBJC:
comment. This helps more easily pinpoint differences from the reference platform implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure any and all changes to CoreFoundation are annotated with a // WINOBJC:

will do, thank you

@bbowman
Copy link
Member

bbowman commented Jan 10, 2017

You have misspelled words in your commit message.

I also don't quite understand the issue and what the fix is here. Can you elaborate?

@@ -182,19 +182,24 @@ static CFURLRef _preferencesDirectoryForUserHostSafetyLevel(CFStringRef userName
#if DEPLOYMENT_TARGET_WINDOWS

CFURLRef url = NULL;
Boolean success = false;

CFMutableStringRef completePath = _CFCreateApplicationRepositoryPath(alloc, CSIDL_APPDATA);
if (completePath) {
// append "Preferences\" and make the CFURL
CFStringAppend(completePath, CFSTR("Preferences\\"));
url = CFURLCreateWithFileSystemPath(alloc, completePath, kCFURLWindowsPathStyle, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completePath [](start = 47, length = 12)

what is this used for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @bbowman .

I also don't quite understand the issue and what the fix is here. Can you elaborate?

The changes I made are fix for the issue #1613 . In my last comment to the issue 1613 I mentioned that I found the solution, which I applied in "_preferencesDirectoryForUserHostSafetyLevel" in CFPreferences.c . But then I changed it to the implementation, which I submitted as PR, because I thought it was more relevant.


CFMutableStringRef completePath = _CFCreateApplicationRepositoryPath(alloc, CSIDL_APPDATA);
if (completePath) {
// append "Preferences\" and make the CFURL
CFStringAppend(completePath, CFSTR("Preferences\\"));
url = CFURLCreateWithFileSystemPath(alloc, completePath, kCFURLWindowsPathStyle, true);
_CFCreateDirectory(CFStringGetCStringPtr(completePath, kCFStringEncodingUTF8)); // WINOBJC: ensure directory exists

char cPath[CFMaxPathSize];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFMaxPathSize [](start = 12, length = 13)

what is this value and is it sufficient for all cases?

Copy link
Contributor Author

@jboich jboich Jan 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @bbowman .
CFMaxPathSize defined in CFInternal.h as following:

`/* Buffer size for file pathname */
#if DEPLOYMENT_TARGET_WINDOWS
#define CFMaxPathSize ((CFIndex)262)
#define CFMaxPathLength ((CFIndex)260)`

I do not know why such value was chosen for Windows target. CFMaxPathSize definition used in many places of CoreFoundation.

@bbowman
Copy link
Member

bbowman commented Jan 10, 2017

Please add unit tests. I'm not sure what the confusion is for testing?
My usual strategy for bug fixes:

  1. Write a test that encapsulates the current bug condition. This should fail.
  2. Fix the product code.
  3. Verify test now passes.

In terms of mocking, we have support for using MOCK_CLASS and similar macros that allow for lambda replacement of function bodies at runtime but that is usually used with an interface pattern to substitute in a mock class in place of the production layers above and below the class under test.

In this case, it seems like you want to verify the URL returned by the function you modified? The key consideration here would be the file system differences on other people's machines and the access rights of the test code. A generally safe assumption is that you can access resources in the current test directory and can access / mess with paths in well known locations like TEMP, etc. Please make sure that the tests use good RAII / similar patterns to ensure consistent cleanup of any machine state that is modified.

@bbowman
Copy link
Member

bbowman commented Jan 10, 2017

🕐

@DHowett-MSFT
Copy link

Note: When we merge, we should update the commit message / take one from the commits in this PR instead of the PR title.

@jboich
Copy link
Contributor Author

jboich commented Jan 10, 2017

Hi, @bbowman .

Please add unit tests. I'm not sure what the confusion is for testing?

I think that I am not familiar with the bridge project as good as you are, so I was asking for advice :)

it seems like you want to verify the URL returned by the function you modified?

Because changes, that I made, are related to issue #1613 I'd like to verify that if completePath contains different set of characters in different languages, then

  1. CFURLGetFileSystemRepresentation returns true and
  2. directory creates successfully by _CFCreateDirectory using character buffer, which filled in by CFURLGetFileSystemRepresentation function (char buffer must not be null)

I will provide you an example of data I mean for 'completePath' later (I don't have them at hand right now)

Thanks for all your feedback!

@jboich
Copy link
Contributor Author

jboich commented Jan 11, 2017

Hi, @bbowman .
So an example of data I get in completePath looks like this
"C:/Users/<name of user>/AppData/Local/Packages/<package name>/LocalState/Preferences"

<name of user> could be in different languages and may contain space.
I point to different languages, because if user name is in English and contain space, issue #1613 did not take place.

<package name> is from dev center and it is in specified format and should not cause any problems when url converts to char array.

_CFCreateDirectory(CFStringGetCStringPtr(completePath, kCFStringEncodingUTF8)); // WINOBJC: ensure directory exists

char cPath[CFMaxPathSize];
if (CFURLGetFileSystemRepresentation(url, true, (unsigned char *)cPath, CFMaxPathSize)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFURLGetFileSystemRepresentation [](start = 5, length = 32)

looking at the implementation for this, it appears that it can return false if the buffer is too small as well. Not a lot we can do here about that though. I think the MaxPath thing is fine for now but it does worry me a for very long names.

@bbowman
Copy link
Member

bbowman commented Jan 13, 2017

@jboich ,

The likely case here is simply a UTF encoding issue where we are getting a different CFstring encoding resulting in a nullptr return from CFStringGetPtr or whatever that call was.
This is probably the interesting bit to test. In terms of testing, its a bit annoying because its a file scoped static function that we want to test. It does look like though that the inner most failing function we could test would be:

_CFPreferencesCreateDomainList

So i'd suggest making a new test in WinObjC\tests\unittests\CoreFoundation\ for CFPreferences that calls _CFPreferencesCreateDomainList and checks the result when the username field is something weird (foreign language and with a space). It would be good to write this test and try it without your fix to see that it fails and then make sure it passes with it.

Thanks!
-Brian


In reply to: 271939958 [](ancestors = 271939958)

@jboich
Copy link
Contributor Author

jboich commented Jan 13, 2017

Hi @bbowman. Thanks for feedback!

that calls _CFPreferencesCreateDomainList and checks the result when the username field is something weird

Assigning different values to username of CFPreferencesCreateDomainList method won't allow us to test what we want, because value of username field is not used to create a path, which will be used then to create url and directory.

Method CF_PRIVATE CFArrayRef _CFPreferencesCreateDomainList(CFStringRef userName, CFStringRef hostName) calls CFURLRef prefDir = _preferencesDirectoryForUserHost(userName, hostName);

_preferencesDirectoryForUserHost calls _preferencesDirectoryForUserHostSafetyLevel

And if you take a look at the implementation of _preferencesDirectoryForUserHostSafetyLevel you will see that username parameter is not taking any part in completePath :
CFMutableStringRef completePath = _CFCreateApplicationRepositoryPath(alloc, CSIDL_APPDATA);

And this guy _CFCreateApplicationRepositoryPath(alloc, CSIDL_APPDATA); returns application data local folder (GetAppDataPath(true))

As far as I understand I need to simulate data returned by GetAppDataPath . Could you give any advice of how to do this?

Kind regards,
Julia

@bbowman
Copy link
Member

bbowman commented Jan 17, 2017

@jboich Sorry you are correct. It does appear that the user name (which we believe is the issue we are fixing) comes in via that GetAppDataPath call.

This makes testing even a bit more difficult which is unfortunate. We have the capability to mock out the WinRT calls that are being made there but we will need to modify the CoreFoundation library and tests to make it more testable. This is nontrivial. Please file a separate issue for that and we can write that test at a later point.

Otherwise I think we are ok accepting your change pending CI build and sign off (I'll go ahead and schedule a build).

Thanks!

@jboich
Copy link
Contributor Author

jboich commented Jan 18, 2017

Hi, @bbowman . Thanks for getting back to me with response.
I did many manual tests and noticed that CFURLGetFileSystemRepresentation sometimes returns odd results. I did look close in the code and noticed that _CFCreateApplicationRepositoryPath returns path in UTF16 encoding and CFURLGetFileSystemRepresentation uses CFStringFileSystemEncoding().
Can I make another PR with the suggestion of the fix?

@bbowman
Copy link
Member

bbowman commented Jan 18, 2017

@jboich That would be great. You can submit a new commit on this one if you feel like it is still part of the same issue seen here.

@bbowman bbowman merged commit 7c86e9a into microsoft:develop Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants