Bug 78655 - [Mac] PasteboardMac.mm build fails
Summary: [Mac] PasteboardMac.mm build fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-14 17:46 PST by Kentaro Hara
Modified: 2012-02-15 22:15 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2012-02-14 17:48 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2012-02-15 03:06 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2012-02-15 03:08 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.27 KB, patch)
2012-02-15 15:48 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2012-02-15 16:44 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-02-14 17:46:26 PST
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
Comment 1 Kentaro Hara 2012-02-14 17:48:32 PST
Created attachment 127090 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-14 18:20:36 PST
Comment on attachment 127090 [details]
Patch

Clearing flags on attachment: 127090

Committed r107770: <http://trac.webkit.org/changeset/107770>
Comment 3 WebKit Review Bot 2012-02-14 18:20:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2012-02-14 18:30:07 PST
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.
Comment 5 Kentaro Hara 2012-02-14 18:46:04 PST
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.
Comment 6 Kentaro Hara 2012-02-15 03:06:16 PST
Reopening to attach new patch.
Comment 7 Kentaro Hara 2012-02-15 03:06:20 PST
Created attachment 127147 [details]
Patch
Comment 8 Kentaro Hara 2012-02-15 03:08:22 PST
Created attachment 127148 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-15 04:13:39 PST
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.
Comment 10 WebKit Review Bot 2012-02-15 04:14:35 PST
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.
Comment 11 Enrica Casucci 2012-02-15 11:06:58 PST
(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.
Comment 12 Darin Adler 2012-02-15 11:20:32 PST
(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.
Comment 13 Darin Adler 2012-02-15 11:21:24 PST
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.
Comment 14 Enrica Casucci 2012-02-15 11:50:41 PST
(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.
Comment 15 Enrica Casucci 2012-02-15 11:52:47 PST
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.
Comment 16 Darin Adler 2012-02-15 12:07:23 PST
(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.
Comment 17 Enrica Casucci 2012-02-15 15:29:58 PST
(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 :-)
Comment 18 Kentaro Hara 2012-02-15 15:37:02 PST
(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.
Comment 19 Kentaro Hara 2012-02-15 15:48:53 PST
Created attachment 127257 [details]
Patch
Comment 20 Kentaro Hara 2012-02-15 15:49:44 PST
(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.
Comment 21 Enrica Casucci 2012-02-15 15:55:27 PST
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];
Comment 22 Kentaro Hara 2012-02-15 16:00:22 PST
(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.
Comment 23 Enrica Casucci 2012-02-15 16:12:04 PST
(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;
Comment 24 Kentaro Hara 2012-02-15 16:19:01 PST
(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.
Comment 25 Enrica Casucci 2012-02-15 16:38:22 PST
(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];
Comment 26 Kentaro Hara 2012-02-15 16:44:03 PST
Created attachment 127274 [details]
Patch

Enrica: Fixed as you pointed out.
Comment 27 Enrica Casucci 2012-02-15 16:49:17 PST
Comment on attachment 127274 [details]
Patch

Looks great! Thanks for adding the test too :-)
Comment 28 WebKit Review Bot 2012-02-15 22:15:12 PST
Comment on attachment 127274 [details]
Patch

Clearing flags on attachment: 127274

Committed r107886: <http://trac.webkit.org/changeset/107886>
Comment 29 WebKit Review Bot 2012-02-15 22:15:18 PST
All reviewed patches have been landed.  Closing bug.