REGRESSION (r199558): WKWebView upload file which name contains Chinese character results in garbled code

I am working on a Mac app, just a simple web application encapsulated in WKWebView.

Here is the code when I tring to upload a file which name contains special character:

-(void)webView:(WKWebView *)webView runOpenPanelWithParameters:(WKOpenPanelParameters *)parameters initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(NSArray<NSURL *> * _Nullable))completionHandler
{
     DEBUG_FUNC(@"");

     NSOpenPanel* openDlg = [NSOpenPanel openPanel];
    [openDlg setCanChooseFiles:YES];
    [openDlg setCanChooseDirectories:NO];
    [openDlg setPrompt:NSLocalizedString(@"Choose", nil)];
    openDlg.allowsMultipleSelection = YES;

    if ( [openDlg runModal] == NSOKButton ) 
    {
        if (completionHandler)
        {
            completionHandler([openDlg URLs]);
        }
    }  
}
When I was using WebView, this way can work, so it should not be a web problem.

But since the project used WKWebView,
for example: file "证书.p12" would be changed to "è¯ä¹¦.p12" after uploaded via WKWebView.

[Alexey Proskuryakov]

Could you please provide a complete test case? Is this problem reproducible for you in Safari with the same HTML content?

Such issues could be caused by not specifying an encoding in your HTML file.

I post a demo on Github:
https://github.com/madao1237/WKWebViewUploadDemo
You can try this demo to do tests.(In reply to Alexey Proskuryakov from comment #1)
> Could you please provide a complete test case? Is this problem reproducible
> for you in Safari with the same HTML content?
> 
> Such issues could be caused by not specifying an encoding in your HTML file.

[Alexey Proskuryakov]

Thank you, this does reproduce, and is a WebKit issue.

Andy looked at this with me, and this should be the fix. Just need to add a regression test.


Index: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm
===================================================================
--- Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm	(revision 216685)
+++ Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm	(working copy)
@@ -365,7 +365,7 @@
 
         Vector<String> filenames;
         for (NSURL *url in URLs)
-            filenames.append(url.fileSystemRepresentation);
+            filenames.append(String::fromUTF8(url.fileSystemRepresentation));
 
         resultListener->chooseFiles(filenames);
     }];

[Radar WebKit Bug Importer]

<rdar://problem/32567454>

(In reply to Alexey Proskuryakov from comment #3)
> Thank you, this does reproduce, and is a WebKit issue.
> 
> Andy looked at this with me, and this should be the fix. Just need to add a
> regression test.
> 
> 
> Index: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm
> ===================================================================
> --- Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm	(revision 216685)
> +++ Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm	(working copy)
> @@ -365,7 +365,7 @@
>  
>          Vector<String> filenames;
>          for (NSURL *url in URLs)
> -            filenames.append(url.fileSystemRepresentation);
> +           
> filenames.append(String::fromUTF8(url.fileSystemRepresentation));
>  
>          resultListener->chooseFiles(filenames);
>      }];


So is there anyway for developer to fix it from outside?
And in which macOS version will this issue be fixed?

I am so anxious because it's really inconvenient, and I want to fix it ASAP in my application too.
Thanks!

[Darin Adler]

(In reply to Alexey Proskuryakov from comment #3)
>          Vector<String> filenames;
>          for (NSURL *url in URLs)
> -            filenames.append(url.fileSystemRepresentation);
> +           
> filenames.append(String::fromUTF8(url.fileSystemRepresentation));

I have said before that if we want to make a String, we probably don’t want use -[NSURL fileSystemRepresentation]. I believe we can use either -[NSURL path] or CFURLCopyFileSystemPath instead to avoid the encoding/decoding process.

Alexey, Andy, did you decide you disagree with me on this?

[Alexey Proskuryakov]

> So is there anyway for developer to fix it from outside?

The only way I can think of is to make a renamed copy of the file in the delegate, one that doesn't have characters outside Latin-1 range. This is not a good workaround, as the file name would have to become different.

> Alexey, Andy, did you decide you disagree with me on this?

I still don't have a complete clarity on which layer is supposed to perform normalization, if any. But this sounds like how the String/NSString based APIs *should* work, so I agree that switching to url.path would be a better fix here.

[Andy Estes]

Created attachment 312085 [details]
Patch

[Build Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RunOpenPanel.mm:46:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


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

[Alexey Proskuryakov]

EWS is red.

[Andy Estes]

(In reply to Alexey Proskuryakov from comment #7)
> > Alexey, Andy, did you decide you disagree with me on this?
> 
> I still don't have a complete clarity on which layer is supposed to perform
> normalization, if any. But this sounds like how the String/NSString based
> APIs *should* work, so I agree that switching to url.path would be a better
> fix here.

I initially proposed Alexey try String::fromUTF8() as a test, but we also discussed whether we should use url.path based on Darin's earlier proposal. I agree that url.path should work here.

[Andy Estes]

Created attachment 312110 [details]
Patch

[Build Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RunOpenPanel.mm:46:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


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

[Andy Estes]

Created attachment 312117 [details]
Patch

[Build Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RunOpenPanel.mm:47:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


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

[Darin Adler]

Looks like RunOpenPanel.mm needs help from AppKitCompatibilityDeclarations.h.

[Andy Estes]

Created attachment 312123 [details]
Patch

[Andy Estes]

(In reply to Darin Adler from comment #16)
> Looks like RunOpenPanel.mm needs help from AppKitCompatibilityDeclarations.h.

In fact the whole test will fail prior to 10.12 because that's when -webView:runOpenPanelWithParameters:initiatedByFrame:completionHandler: was introduced. I've skipped the test prior to 10.12.

[Build Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RunOpenPanel.mm:47:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


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

[WebKit Commit Bot]

Comment on attachment 312123 [details]
Patch

Clearing flags on attachment: 312123

Committed r217865: <http://trac.webkit.org/changeset/217865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[liaokobe]

It would be highly appreciated to know which macOS version will have this patch. The latest version is still 10.12.6 (16G29).