Bug 172849 - REGRESSION (r199558): WKWebView upload file which name contains Chinese character results in garbled code
Summary: REGRESSION (r199558): WKWebView upload file which name contains Chinese chara...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Other
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-02 00:24 PDT by johnhu456
Modified: 2017-08-29 19:01 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.73 KB, patch)
2017-06-06 10:29 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2017-06-06 13:05 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2017-06-06 13:50 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2017-06-06 14:49 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description johnhu456 2017-06-02 00:24:30 PDT
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.
Comment 1 Alexey Proskuryakov 2017-06-04 09:30:22 PDT
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.
Comment 2 johnhu456 2017-06-04 18:41:23 PDT
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.
Comment 3 Alexey Proskuryakov 2017-06-05 09:30:21 PDT
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);
     }];
Comment 4 Radar WebKit Bug Importer 2017-06-05 09:31:07 PDT
<rdar://problem/32567454>
Comment 5 johnhu456 2017-06-05 19:57:06 PDT
(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!
Comment 6 Darin Adler 2017-06-06 09:18:26 PDT
(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?
Comment 7 Alexey Proskuryakov 2017-06-06 09:58:09 PDT
> 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.
Comment 8 Andy Estes 2017-06-06 10:29:14 PDT
Created attachment 312085 [details]
Patch
Comment 9 Build Bot 2017-06-06 10:30:11 PDT
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.
Comment 10 Alexey Proskuryakov 2017-06-06 12:33:02 PDT
EWS is red.
Comment 11 Andy Estes 2017-06-06 12:56:17 PDT
(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.
Comment 12 Andy Estes 2017-06-06 13:05:52 PDT
Created attachment 312110 [details]
Patch
Comment 13 Build Bot 2017-06-06 13:06:54 PDT
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.
Comment 14 Andy Estes 2017-06-06 13:50:26 PDT
Created attachment 312117 [details]
Patch
Comment 15 Build Bot 2017-06-06 13:51:45 PDT
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.
Comment 16 Darin Adler 2017-06-06 14:41:14 PDT
Looks like RunOpenPanel.mm needs help from AppKitCompatibilityDeclarations.h.
Comment 17 Andy Estes 2017-06-06 14:49:10 PDT
Created attachment 312123 [details]
Patch
Comment 18 Andy Estes 2017-06-06 14:51:05 PDT
(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.
Comment 19 Build Bot 2017-06-06 14:52:09 PDT
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.
Comment 20 WebKit Commit Bot 2017-06-06 16:04:52 PDT
Comment on attachment 312123 [details]
Patch

Clearing flags on attachment: 312123

Committed r217865: <http://trac.webkit.org/changeset/217865>
Comment 21 WebKit Commit Bot 2017-06-06 16:04:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 liaokobe 2017-08-29 19:01:27 PDT
It would be highly appreciated to know which macOS version will have this patch. The latest version is still 10.12.6 (16G29).