Bug 78655 – [Mac] PasteboardMac.mm build fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kentaro Hara]

Although the bots have been working fine, PasteboardMac.mm build fails in our local Mac environments due to an uninitialized variable.

    /Users/haraken/WebKit/Source/WebCore/platform/mac/PasteboardMac.mm:322: warning: 'string' may be used uninitialized in this function

[Kentaro Hara]

Created attachment 127090 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127090 [details]
Patch

Clearing flags on attachment: 127090

Committed r107770: <http://trac.webkit.org/changeset/107770>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 127090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review

> Source/WebCore/platform/mac/PasteboardMac.mm:322
> +    NSString *string = nil;

This is not the right fix.

The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug.

[Kentaro Hara]

Comment on attachment 127090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review

>> Source/WebCore/platform/mac/PasteboardMac.mm:322
>> +    NSString *string = nil;
> 
> This is not the right fix.
> 
> The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug.

I got it. Let me fix it.

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 127147 [details]
Patch

[Kentaro Hara]

Created attachment 127148 [details]
Patch

[WebKit Review Bot]

Attachment 127147 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
	M	LayoutTests/fast/forms/radio-nested-labels.html
	M	LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 127148 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
	M	LayoutTests/fast/forms/radio-nested-labels.html
	M	LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

(In reply to comment #5)
> (From update of attachment 127090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review
> 
> >> Source/WebCore/platform/mac/PasteboardMac.mm:322
> >> +    NSString *string = nil;
> > 
> > This is not the right fix.
> > 
> > The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug.
> 
> I got it. Let me fix it.

Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct.

[Darin Adler]

(In reply to comment #11)
> Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct.

Here’s the old code:

    for (size_t i = 0; i < pathnames.size(); i++)
        string = [string length] ? @"\n" + pathnames[i] : pathnames[i];
    string = [string precomposedStringWithCanonicalMapping];

That code will produce a string containing the last pathname in the vector, possibly with a newline prepended. It will not produce a string with multiple pathnames in it.

[Darin Adler]

Comment on attachment 127148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127148&action=review

> Source/WebCore/platform/mac/PasteboardMac.mm:345
> +                string = [NSString stringWithFormat:@"%@\n%@", string, pathname];

It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end.

[Enrica Casucci]

(In reply to comment #12)
> (In reply to comment #11)
> > Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct.
> 
> Here’s the old code:
> 
>     for (size_t i = 0; i < pathnames.size(); i++)
>         string = [string length] ? @"\n" + pathnames[i] : pathnames[i];
>     string = [string precomposedStringWithCanonicalMapping];
> 
> That code will produce a string containing the last pathname in the vector, possibly with a newline prepended. It will not produce a string with multiple pathnames in it.

You're absolutely right, I missed that. The correct code is:

string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i];

I think that adding the nil initialization is ok, as long as the other fix goes in.

[Enrica Casucci]

Comment on attachment 127148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127148&action=review

>> Source/WebCore/platform/mac/PasteboardMac.mm:345
>> +                string = [NSString stringWithFormat:@"%@\n%@", string, pathname];
> 
> It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end.

I think it is enough to change the code in the original for loop to 
string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i]
and of course leave the initialization to nil.

[Darin Adler]

(In reply to comment #15)
> string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i]

OK, but that still won’t work, and might not even compile, because we can’t just add two NSString * together using the + operator.

We need test coverage so we can know this is working code.

[Enrica Casucci]

(In reply to comment #16)
> (In reply to comment #15)
> > string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i]
> 
> OK, but that still won’t work, and might not even compile, because we can’t just add two NSString * together using the + operator.
> 
> We need test coverage so we can know this is working code.

You're right it does not.
This code is ok:

        for (size_t i = 0; i < pathnames.size(); i++)
            string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i];

Kentaro, if you want I can take care of this, since I'm the one who introduced the bug :-)

[Kentaro Hara]

(In reply to comment #17)
> Kentaro, if you want I can take care of this, since I'm the one who introduced the bug :-)

Good morning:-) I'll upload a patch in minutes.

[Kentaro Hara]

Created attachment 127257 [details]
Patch

[Kentaro Hara]

(In reply to comment #13)
> It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end.

Darin: Uploaded a patch using StringBuilder.

[Enrica Casucci]

Comment on attachment 127257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127257&action=review

> Source/WebCore/platform/mac/PasteboardMac.mm:350
> +        }

I don't think you need all these changes. 
The only line you need to change in the one in the original for loop to the following:
      string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i];

[Kentaro Hara]

(In reply to comment #21)
> I don't think you need all these changes. 
> The only line you need to change in the one in the original for loop to the following:
>       string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i];

As Darin pointed out, using StringBuilder would be better for performance. In the above code, the conversion from String to NSString happens at every concatenation.

[Enrica Casucci]

(In reply to comment #22)
> (In reply to comment #21)
> > I don't think you need all these changes. 
> > The only line you need to change in the one in the original for loop to the following:
> >       string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i];
> 
> As Darin pointed out, using StringBuilder would be better for performance. In the above code, the conversion from String to NSString happens at every concatenation.

That is right. Then, since we only need to deal with the NSString at the very end I would suggest:

        String pathnamesString;
        for (size_t i = 0; i < pathnames.size(); i++)
            pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i];
        string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping];
        if (string != nil)
            return string;

[Kentaro Hara]

(In reply to comment #23)
> That is right. Then, since we only need to deal with the NSString at the very end I would suggest:
> 
>         String pathnamesString;
>         for (size_t i = 0; i < pathnames.size(); i++)
>             pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i];
>         string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping];
>         if (string != nil)
>             return string;

Using StringBuilder for String concatenation is also important for performance.

[Enrica Casucci]

(In reply to comment #24)
> (In reply to comment #23)
> > That is right. Then, since we only need to deal with the NSString at the very end I would suggest:
> > 
> >         String pathnamesString;
> >         for (size_t i = 0; i < pathnames.size(); i++)
> >             pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i];
> >         string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping];
> >         if (string != nil)
> >             return string;
> 
> Using StringBuilder for String concatenation is also important for performance.

Ok, then I'd prefer this:

        StringBuilder builder;
        for (size_t i = 0; i < pathnames.size(); i++)
            builder.append(i ? "\n" + pathnames[i] : pathnames[i]);
        string = [(NSString *)builder.toString() precomposedStringWithCanonicalMapping];

[Kentaro Hara]

Created attachment 127274 [details]
Patch

Enrica: Fixed as you pointed out.

[Enrica Casucci]

Comment on attachment 127274 [details]
Patch

Looks great! Thanks for adding the test too :-)

[WebKit Review Bot]

Comment on attachment 127274 [details]
Patch

Clearing flags on attachment: 127274

Committed r107886: <http://trac.webkit.org/changeset/107886>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.