Fix deprecated warning in GetMetadataForFile.c

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
OS Integration
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nomis101, Assigned: Nomis101)

Tracking

unspecified
Thunderbird 53.0
Unspecified
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8762311 [details] [diff] [review]
Patch

This patch will fix the following -Wdeprecated-declarations warning:

/mail/components/search/mdimporter/GetMetadataForFile.c:33:30: warning: 
      'CFPropertyListCreateFromStream' is deprecated: first deprecated in OS X
      10.10 - Use CFPropertyListCreateWithStream instead.
      [-Wdeprecated-declarations]
  CFPropertyListRef ticket = CFPropertyListCreateFromStream(kCFAllocatorDefault,
                             ^
Attachment #8762311 - Flags: review?(Pidgeot18)

Comment 1

2 years ago
Comment on attachment 8762311 [details] [diff] [review]
Patch

Hi. I'm not sure jcranmer will respond in the near future. Let's try rkent to forward as appropriate.

Do you know the new function will do the right thing? Or do you just replace the names as the deprecation warning suggests?
Attachment #8762311 - Flags: review?(rkent)
(Assignee)

Comment 2

2 years ago
(In reply to :aceman from comment #1)
> Do you know the new function will do the right thing? Or do you just replace
> the names as the deprecation warning suggests?
Both, and I tested locally that it still will build.
Here is the documentation: https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFPropertyListRef/#//apple_ref/c/func/CFPropertyListCreateFromStream

https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFPropertyListRef/#//apple_ref/c/func/CFPropertyListCreateWithStream

Comment 3

2 years ago
I was hoping we could find someone who routinely work on OSX to review this, but let me try to comment.

The difference between CFPropertyListCreateWithStream and CFPropertyListCreateFromStream as far as I can tell is a different type for the error return.

CFPropertyListCreateFromStream has "errorString - On return, NULL if the conversion is successful, otherwise a string that describes the nature of the error. "

CFPropertyListCreateWithStream has " error - If this parameter is non-NULL, if an error occurs, on return this will contain a CFError error describing the problem. "

I don't know much about the OSX libraries, but this seems like a type mismatch. I don't know whether these classes have some sort of automatic conversion. Can you convince me that this works, or "it compiles" is the only test?
(Assignee)

Comment 4

2 years ago
(In reply to Kent James (:rkent) from comment #3)
> I don't know much about the OSX libraries, but this seems like a type
> mismatch. I don't know whether these classes have some sort of automatic
> conversion. Can you convince me that this works, or "it compiles" is the
> only test?
My template for this was another patch I found on the web, I think it was this one: https://git.samba.org/samba.git/?p=abartlet/lorikeet-heimdal.git/.git;a=patch;h=37986474004b1f7c149522a6fa6b7be198f72094
But there the errorString is set to NULL from the beginning.

"Works" means, that if an error occurs it does the right thing? Actually, because the mdimporter plugin is currently not working (Bug 1011449), I don't know how test this. So, "it compiles" is indeed my only test so far. But if you (or anybody else) has suggestions how I can further test that it "works", I can try that.

Comment 5

2 years ago
Check this http://svn.gna.org/svn/gnustep/libs/corebase/branches/corebase-0_1/Source/CFPropertyList.m

  CFErrorRef err = NULL;
  
  CFPropertyListRef ret =
    CFPropertyListCreateWithStream (allocator, stream, streamLength,
      mutabilityOption, format, &err);
  if (err != NULL)
    {
      if (errorString != NULL)
        {
          *errorString = CFErrorCopyDescription (err);
        }
      CFRelease (err);
    }

That is a bit different that what you are doing.

I think it would be good to better understand how this error return is supposed to be used. We could also consider just removing its use (after all, it is only used to printf). Can you see if non-null err is enough to check for the existence on an error?
(Assignee)

Comment 7

2 years ago
Created attachment 8763330 [details] [diff] [review]
Different approach

(In reply to Nomis101 from comment #6)
> Would it be OK to change it to the same format like there:
> http://hg.mozilla.org/mozilla-central/file/tip/modules/libmar/verify/
> MacVerifyCrypto.cpp#l58
> and there:
> http://hg.mozilla.org/mozilla-central/file/tip/security/sandbox/mac/Sandbox.
> mm#l71
> ?

Like this. This does also fix the warning for me and does not produce any new one.
Attachment #8763330 - Flags: feedback?(rkent)

Comment 8

2 years ago
Comment on attachment 8763330 [details] [diff] [review]
Different approach

Since Javi commented on the other patch, perhaps he could give feedback here as well?

Removing error detection is a change in behavior, it would be better to understand the correct replacement code instead of ignoring the error response.
Attachment #8763330 - Flags: feedback?(rkent) → feedback?(leofigueres)

Comment 9

2 years ago
Comment on attachment 8763330 [details] [diff] [review]
Different approach

Review of attachment 8763330 [details] [diff] [review]:
-----------------------------------------------------------------

I am thinking the same as :rkent about the removal of the errorString checking after calling the function.

Furthermore, the *format* parameter is passed as NULL, so the allocation of the object is not needed as it is not used anywhere in the code.

That same comment is also affecting the patch which is currently set to be reviewed.

So I think that a better fix for this bug would be a mixture of both patches but leaving the errorString checking, as :rkent suggested.

The same commentaries about the fact that the function which replaces the deprecated one requires OS X 10.6 as a minimum. However, I saw that since may we have removed compatibility for OS X 10.8- so this should not be a problem.
Attachment #8763330 - Flags: feedback?(leofigueres) → feedback-

Comment 10

2 years ago
(In reply to Kent James (:rkent) from comment #3)
> The difference between CFPropertyListCreateWithStream and
> CFPropertyListCreateFromStream as far as I can tell is a different type for
> the error return.
> 
> CFPropertyListCreateFromStream has "errorString - On return, NULL if the
> conversion is successful, otherwise a string that describes the nature of
> the error. "

That was the deprecated function. In case of failure it returned a string in that reference.

> 
> CFPropertyListCreateWithStream has " error - If this parameter is non-NULL,
> if an error occurs, on return this will contain a CFError error describing
> the problem. "

This is the recommended function to use since OS X 10.6, The programmer could pass NULL if we don't cre about any failure. If we pase a CFErrorRef, then the function will fill the object CFError referenced with the information. The new function -method, sorry- can give us more information: https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFErrorRef/index.html#//apple_ref/doc/constant_group/Keys_for_the_user_info_dictionary

> 
> I don't know much about the OSX libraries, but this seems like a type
> mismatch. I don't know whether these classes have some sort of automatic
> conversion. Can you convince me that this works, or "it compiles" is the
> only test?

Not a type mismatch. The recommended method fills an object. The deprecated one filled an object also, but that object could only contain one string.

Comment 11

2 years ago
Comment on attachment 8762311 [details] [diff] [review]
Patch

Review of attachment 8762311 [details] [diff] [review]:
-----------------------------------------------------------------

The *WithStream method accepts NULL as the format parameter. We are not using it, either. This way we would save the allocation of the CFPropertyListFormat object.
Attachment #8762311 - Flags: feedback-
(Assignee)

Comment 12

2 years ago
Thanks for your suggestions, this is getting a bit more difficult now. Will try that over the weekend and hopefully come up with an improved patch.

Comment 13

2 years ago
Comment on attachment 8762311 [details] [diff] [review]
Patch

I'm removing the review request since the last response was a plan to investigate suggested changes.
Attachment #8762311 - Flags: review?(rkent)
(Assignee)

Comment 14

2 years ago
(In reply to Javi Rueda from comment #9)

> So I think that a better fix for this bug would be a mixture of both patches
> but leaving the errorString checking, as :rkent suggested.
If you say "mixture", do you than mean, leaving the errorString checking (like in the first patch), but passing NULL to the *format* parameter (like in the second patch)?

Comment 15

2 years ago
Sorry for the delay in my response, Nomis101.

I meant use the function which Apple is recommending to use, but pass NULL instead of a pointer to a format object, removing the creation of the /format/ object, as we are not using it.

About the CFError parameter, althought the code is only printingf it

(In reply to Nomis101 from comment #4)
> 
> "Works" means, that if an error occurs it does the right thing? Actually,
> because the mdimporter plugin is currently not working (Bug 1011449), I
> don't know how test this. So, "it compiles" is indeed my only test so far.
> But if you (or anybody else) has suggestions how I can further test that it
> "works", I can try that.

providing that information could help debuging that bug you referenced in the above comment. Just take into consideration that a CFError object is not a string so you should browse into it to get the same string as errorString we were using before on the deprecated function.
(Assignee)

Comment 16

2 years ago
Created attachment 8777926 [details] [diff] [review]
Maybe?

I found some time, to write a new patch. I need to say, because my programming skills are limited and I therefore was not able to fully understand your suggestions, I've tried to make a new patch by copy&paste of the code from #5 and the other ones, until my feeling told me the patch could hopefully do what you suggested.
Attachment #8762311 - Attachment is obsolete: true
Attachment #8763330 - Attachment is obsolete: true
Attachment #8762311 - Flags: review?(Pidgeot18)
Attachment #8777926 - Flags: feedback?(leofigueres)

Comment 17

2 years ago
Comment on attachment 8777926 [details] [diff] [review]
Maybe?

Review of attachment 8777926 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for this new patch, Nomis101. Very often my explanations are misunderstood, it is very likely this one was also the case. If you are unsure about something in the future about any of my comments, don't hesitate to ask me wit a new answer in Bugzilla.

I hope this time my comments are much more clear.

::: mail/components/search/mdimporter/GetMetadataForFile.c
@@ +27,5 @@
>    CFURLRef fileURL = CFURLCreateWithFileSystemPath(kCFAllocatorDefault, pathToFile, kCFURLPOSIXPathStyle, false);
>    CFReadStreamRef stream = CFReadStreamCreateWithFile(kCFAllocatorDefault, fileURL);
>    CFReadStreamOpen(stream);
>  
>    CFPropertyListFormat format;

Remove this. You are not using it when calling CFPropertyListCreateWithStream method. That was what I was suggesting on my prior comment.

@@ +40,3 @@
>                               );
> +   if (err != NULL)
> +   {

Move the errorString declaration here.

If we get here is because something happened that caused an error, so no need to check errorString != NULL.

Just declare and assign it at the same line, without checking it against NULL, as this object will be populated if we have had an error.

if (err != NULL) {
  /* get the error description */
  /* do whatever we would like to do with that string
     maybe the same we were doing on the original code */
  /* release err */
  /* return ticket */ (Although this return clase could be moved after this if..else and return it even the method produced an error. Check any code which calls this lines you are modifying to see if they check for the possibility that /ticket/ could be NULL)
}
Attachment #8777926 - Flags: feedback?(leofigueres) → feedback-
(Assignee)

Comment 18

2 years ago
(In reply to Javi Rueda from comment #17)
Sorry, for misunderstanding you and thanks for still helping me. It is not your explanation, it is my limited programming skills.

> Move the errorString declaration here.
So, you mean I should move
CFStringRef *errorString = NULL;
after the '{'?

> Although this return clase could be moved after this if..else and return it even the method produced an error.
You mean, I should move the return right before the comment about kMDItemTextContent or before 'success = TRUE'?

> Check any code which calls this lines you are modifying to see if they check for the possibility that /ticket/ could be NULL
Maybe I need to ask you also about that, but first I want to fix your other comments.

Comment 19

2 years ago
Hi, nomis101.

Before answering your questings I would like to clarify that what you should remove was only the line which declared /format/, not the previous ones on my annotation, which were added by Bugzilla.

(In reply to Nomis101 from comment #18)
> (In reply to Javi Rueda from comment #17)
> > Move the errorString declaration here.
> So, you mean I should move
> CFStringRef *errorString = NULL;
> after the '{'?

Yes. errorString is used only inside the if-block so no need to be declared outside of it.

> > Although this return clase could be moved after this if..else and return it even the method produced an error.
> You mean, I should move the return right before the comment about
> kMDItemTextContent or before 'success = TRUE'?

Comment 17 included a block of code at the end, which you are referring in this question. That block of code is pseudo-code. The if-block should end with "return ticket;", so, it should read:

if() {
  (blah blah blah blah)
  return ticket;
}

In my annotation I was saying that because an error was detected -remember that we are inside an if-block that will execute when something went wrong- so ticket would contain useless data. So maybe that "return ticket;" could be moved after the error checking block.

What I was in doubt is that code which is calling the code you are modifying maybe will not check if "ticket" -the value we are returning- contains valid data.

> > Check any code which calls this lines you are modifying to see if they check for the possibility that /ticket/ could be NULL
> Maybe I need to ask you also about that, but first I want to fix your other
> comments.

Good. That was what I was trying to explain in my previous lines. When you call a method and that method returns some data -in the form of an object-, the calling code is responsible to check if the response is valid or not. Very often, an invalid response is "NULL" when the CALLED method was unable to do its job.

- Hey, get some tweets
* Ok. Please, wait for the response.

[starts to check some tweets, images and whatever is needed]

(Internet connectivity is lost)

[Oh, oh. I was unable to get everything that I should send as a response. Let's just return NULL]
* Here is your tweets: NULL
- Oh, thank you very much.

[App crashes]

- Oh, no! I was trying to show an image on the screen, but the app where I am included just crashed. Next time I will check if there is an image to be shown.

I hope that illustrates what should be done. Right now I have not too much time -although it is more a technical problem- to help you in that part of the code.

You could just look in DXR and maybe create a new bug report if you are unable to fix in the case calling code is not checking for invalid returned data. Sometimes developers marked it with a comment /* FIX ME */ and a brief explanation of the problem or just a link to these comments.

I am not a reviewer so I am unsure which would be the better way: try to add error checking code before landing the patch or just ask if someone could do it in another bug-report or even here.

But don't hesitate to ask me again, I am not saying that I don't want to help. I'm saying that right now I am unable to look into that code. Probably this will change this week.

Comment 20

2 years ago
Forget about what I said on "return ticket;" and checking the NULL value.

Last line of the if-block should be "success = FALSE;". The following else-block will not execute, then all what we opened will be closed (https://dxr.mozilla.org/comm-central/source/mail/components/search/mdimporter/GetMetadataForFile.c#81) and calling code will get FALSE (line 84), indicating that code you are modifying was unable to do its job. The errorString will contain the reason. But checking that cause has nothing to do with this bug.

Sorry for the confusion.
(Assignee)

Comment 21

2 years ago
Thank you very much for your long explanation, I will try this as soon as I will find some more time.
(Assignee)

Comment 22

2 years ago
Created attachment 8820805 [details] [diff] [review]
patch v4

I've found some time. So I created a new patch. I really hope I've now correctly included your suggestions. If you find some time maybe you could have a look at it.
I tested that it builds with this changes, it does not introduce any new error or warning.
Attachment #8777926 - Attachment is obsolete: true
Attachment #8820805 - Flags: feedback?(leofigueres)

Comment 23

2 years ago
Comment on attachment 8820805 [details] [diff] [review]
patch v4

Review of attachment 8820805 [details] [diff] [review]:
-----------------------------------------------------------------

Some of my comments in this feedback needs to add spaces at the beggining of the line. Take it into account when attaching the new one.

When you were checking against errorString being NULL -which will always be, as you were not getting the localized string from err- only the first line would run, as there was any explicit block of code. If there is a localized error string, both printf statements should be run. Your code will only run the first one, the one which said "failed creating property list from stream". The other one will always be executed, no matter if there was a localized error string. And that was not good :) My suggestion fixes this.

When you tested this, you probably didn't notice it, as there probably were any error, so execution will go to the ELSE instead to the "if (err != NULL)".

Also, your patch uses { in new lines. I have added them in the same line in my suggestion. Please, fix this in your next attachment to keep the coding style. Sorry about that, but it makes my annotation easier to understand.

::: mail/components/search/mdimporter/GetMetadataForFile.c
@@ +38,4 @@
>                               );
> +   if (err != NULL)
> +   {
> +       CFStringRef *errorString = NULL;

CFStringRef *errorString = CFErrorCopyDescription(err);

@@ +38,5 @@
>                               );
> +   if (err != NULL)
> +   {
> +       CFStringRef *errorString = NULL;
> +       if (errorString != NULL)

if (errorString != NULL) {
  // There is an error description that would help us when debugging, if needed

@@ +43,2 @@
>      printf("failed creating property list from stream\n");
>      printf("error = %s\n", (const char*) errorString);

printf("error = %s\n", (const char*) errorString);
}

@@ +46,2 @@
>      success = FALSE;
>    } 

Please remove this space at the end of the line

@@ +51,5 @@
>      value = CFDictionaryGetValue(ticket, kMDItemTitle);
>       if (value)
>       {
>         CFDictionarySetValue(attributes, kMDItemTitle, value);
>       }    

Please remove these spaces also at the end of the line. Not your fault, but as you are modifying this code and there should be no spaces at the end of any line, there is no damage in doing it :)
Attachment #8820805 - Flags: feedback?(leofigueres) → feedback-
(Assignee)

Comment 24

2 years ago
Created attachment 8820923 [details] [diff] [review]
patch v5

Because you said there should be no spaces at the end of any line, I tried to remove all of them in this file. Thing is, with this new patch I'm now getting a warning I've did not see before:

/Volumes/Developer/comm-aurora/mail/components/search/mdimporter/GetMetadataForFile.c:41:21: warning: 
      incompatible pointer types initializing 'CFStringRef *' (aka 'const struct
      __CFString **') with an expression of type 'CFStringRef' (aka 'const
      struct __CFString *') [-Wincompatible-pointer-types]
       CFStringRef *errorString = CFErrorCopyDescription(err);
                    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
Attachment #8820805 - Attachment is obsolete: true
Attachment #8820923 - Flags: feedback?(leofigueres)

Updated

2 years ago
Assignee: nobody → Nomis101

Comment 25

2 years ago
Comment on attachment 8820923 [details] [diff] [review]
patch v5

Review of attachment 8820923 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for removing those spaces at the end of those lines.

The warning you were getting was because of me. It was my fault.

Here I gave you some more changes that would be needed.

When adding those suggestions the indentation wasn't preserved. However, that is something quite important and most reviewers will not grant your changes if that rule is not complied. It is quite simple: always indent lines with 2 spaces more than previous line, if in a new block. Never with TABS.

Just take a look and you will see that most of the files in Mozilla repositories will do it this way.

::: mail/components/search/mdimporter/GetMetadataForFile.c
@@ +38,4 @@
>                               );
> +   if (err != NULL)
> +   {
> +       CFStringRef *errorString = CFErrorCopyDescription(err);

My fault. It should be:

CFStringRef errorString = CFErrorCopyDescription(err);

Indent line with 2 spaces.

@@ +40,5 @@
> +   {
> +       CFStringRef *errorString = CFErrorCopyDescription(err);
> +       if (errorString != NULL) {
> +       printf("failed creating property list from stream\n");
> +       printf("error = %s\n", (const char*) errorString);

Those two printf should be indented with 2 spaces also, as they are in a block. Also, move the { to a new line, instead, as it is what it has been used in this file.

@@ +42,5 @@
> +       if (errorString != NULL) {
> +       printf("failed creating property list from stream\n");
> +       printf("error = %s\n", (const char*) errorString);
> +       }
> +       CFRelease (err);

Indent correctly: only 2 spaces

@@ +57,5 @@
>       value = CFDictionaryGetValue(ticket, kMDItemTextContent);
>       if (value)
>       {
>         CFDictionarySetValue(attributes, kMDItemTextContent, value);
> +

Thank you for removing the spaces here. But this line is empty and not needed. Could you please remove it?

Also notice the indentation. This one is correct: only two spaces (NEVER tabs) from previous line indentation. This is the way lines 43 and 44 should look :) The same with the opening bracket. Most times it should be at the same line.

However here it is on a new line, and that's the way it has been used in this file. To avoid more problems with the review, it is better to just keep using what has been used in the file you are modifying.
Attachment #8820923 - Flags: feedback?(leofigueres) → feedback-

Comment 26

2 years ago
Oh, wait and now those printf would probably need to be changed as errorString has been re-definied.

Wait. I am compiling patch right now.

Comment 27

2 years ago
I have tested and now it builds.

What you need to change now is just remove the * from the errorString.

So, just to clarify: comment 25 is what should be done now. And it seems it is going to be last one :)
(Assignee)

Comment 28

2 years ago
(In reply to Javi Rueda from comment #25)
It is quite simple: always indent
> lines with 2 spaces more than previous line, if in a new block. Never with
> TABS.
OK. Thats the difference from patches for mozilla-central, they always want TABS instead of spaces.


(In reply to Javi Rueda from comment #27)
> I have tested and now it builds.
> 
> What you need to change now is just remove the * from the errorString.
> 
> So, just to clarify: comment 25 is what should be done now. And it seems it
> is going to be last one :)
Thank you so much for your help! Than I will do this and ask for review than.
(Assignee)

Comment 29

2 years ago
Created attachment 8821133 [details] [diff] [review]
patch v6

Setting rkent for reviewing this again, because I did so in the beginning. I know, you are not routinely working on OSX, but maybe because Javi Rueda helped me with the patch, this will make things easier for you.
Attachment #8820923 - Attachment is obsolete: true
Attachment #8821133 - Flags: review?(rkent)

Updated

2 years ago
Attachment #8821133 - Flags: feedback+
(Assignee)

Comment 31

2 years ago
(In reply to Kent James (:rkent) from comment #30)
> Try server run (in progress): 
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=fcc4252defac5934c5dae4e1b9ebac70e25b8e24

Did it finish successfully? I don't find it in that list...

Comment 32

2 years ago
(In reply to Nomis101 from comment #31)
> (In reply to Kent James (:rkent) from comment #30)
> > Try server run (in progress): 
> > https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> > central&revision=fcc4252defac5934c5dae4e1b9ebac70e25b8e24
> 
> Did it finish successfully? I don't find it in that list...

I think that comment 30 wasn't for this bug. As you can see, your code is not yet in https://dxr.mozilla.org/comm-central/source/mail/components/search/mdimporter/GetMetadataForFile.c the repository and Kent hasn't review possitively the patch.
(Assignee)

Comment 33

2 years ago
(In reply to Javi Rueda from comment #32)
> (In reply to Nomis101 from comment #31)
> > (In reply to Kent James (:rkent) from comment #30)
> > > Try server run (in progress): 
> > > https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> > > central&revision=fcc4252defac5934c5dae4e1b9ebac70e25b8e24
> > 
> > Did it finish successfully? I don't find it in that list...
> 
> I think that comment 30 wasn't for this bug. As you can see, your code is
> not yet in
> https://dxr.mozilla.org/comm-central/source/mail/components/search/
> mdimporter/GetMetadataForFile.c the repository and Kent hasn't review
> possitively the patch.

Its just the try server, not comm-central. But you could be right, that comment 30 wasn't for this bug.

Comment 34

2 years ago
Comment on attachment 8821133 [details] [diff] [review]
patch v6

Review of attachment 8821133 [details] [diff] [review]:
-----------------------------------------------------------------

I was trying to do a try server run for this, but apparently I did not get it correct. Hard to see the OSX X errors anyway since there are a bunch of them there. So this has had enough attention, let's just accept it.
Attachment #8821133 - Flags: review?(rkent) → review+
(Assignee)

Comment 35

2 years ago
Cool, thanks! :-)
Keywords: checkin-needed

Comment 36

2 years ago
thanks
https://hg.mozilla.org/comm-central/rev/3b76c1481e2f27b7ae3f6b57de5af386ff381c6e
You removed some trailing spaces but not all. I killed the remaining ones before landing this.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in before you can comment on or make changes to this bug.