Bug 188191

Summary: [WTF] Rename String::format to String::deprecatedFormat
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: Web Template FrameworkAssignee: Tomas Popela <tpopela>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ap, bburg, commit-queue, darin, ews-watchlist, jbedard, joepeck, lforschler, mitz, realdawei
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=192742
https://bugs.webkit.org/show_bug.cgi?id=192746
https://bugs.webkit.org/show_bug.cgi?id=30342
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2 none

Description Tomas Popela 2018-07-31 01:10:47 PDT
From bug 187955:

We should rename String::format to String::deprecatedFormat and start phasing it out. Clients can and should use string concatenation, clearly invokable as makeString or possibly just with the + operator, instead. More efficient, likely safer. A few clients might be using advanced features of printf, but even those could probably be added to the concatenation.
Comment 1 Tomas Popela 2018-07-31 03:43:40 PDT
Created attachment 346150 [details]
Patch
Comment 2 EWS Watchlist 2018-07-31 03:45:58 PDT
Attachment 346150 [details] did not pass style-queue:


ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:170:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:173:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:175:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 3 in 116 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EWS Watchlist 2018-07-31 05:22:18 PDT
Comment on attachment 346150 [details]
Patch

Attachment 346150 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8709061

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2018-07-31 05:22:19 PDT
Created attachment 346154 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 5 EWS Watchlist 2018-07-31 06:34:54 PDT
Comment on attachment 346150 [details]
Patch

Attachment 346150 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8709327

New failing tests:
legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html
Comment 6 EWS Watchlist 2018-07-31 06:35:05 PDT
Created attachment 346164 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 7 Darin Adler 2018-07-31 09:15:34 PDT
Do we understand what happened with the iOS simulator tests?
Comment 8 Darin Adler 2018-07-31 09:16:15 PDT
(In reply to Build Bot from comment #2)
> ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:170:  Missing spaces
> around /  [whitespace/operators] [3]
> ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:173:  Missing spaces
> around /  [whitespace/operators] [3]
> ERROR: Source/WebCore/html/FTPDirectoryDocument.cpp:175:  Missing spaces
> around /  [whitespace/operators] [3]
> Total errors found: 3 in 116 files

Should probably just add the spaces since we are touching these lines.
Comment 9 Tomas Popela 2018-08-01 04:21:24 PDT
Created attachment 346269 [details]
Patch
Comment 10 EWS Watchlist 2018-08-01 06:03:11 PDT
Comment on attachment 346269 [details]
Patch

Attachment 346269 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8722562

Number of test failures exceeded the failure limit.
Comment 11 EWS Watchlist 2018-08-01 06:03:13 PDT
Created attachment 346274 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 12 WebKit Commit Bot 2018-08-01 19:11:55 PDT
Comment on attachment 346269 [details]
Patch

Clearing flags on attachment: 346269

Committed r234489: <https://trac.webkit.org/changeset/234489>
Comment 13 WebKit Commit Bot 2018-08-01 19:11:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-08-01 20:33:45 PDT
<rdar://problem/42840464>
Comment 15 Dawei Fenton (:realdawei) 2018-08-02 08:32:18 PDT
(In reply to WebKit Commit Bot from comment #12)
> Comment on attachment 346269 [details]
> Patch
> 
> Clearing flags on attachment: 346269
> 
> Committed r234489: <https://trac.webkit.org/changeset/234489>

after this revision I'm seeing 50 Layout test crashes and 63 unit test failures on iOS

Layout Test output:
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/6557/steps/layout-test/logs/stdio

API test output:
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/6557/steps/run-api-tests/logs/stdio
Comment 16 Dawei Fenton (:realdawei) 2018-08-02 08:39:30 PDT
here's output from a debug crash:

ASSERTION FAILED: dlopen(/System/Library/Frameworks/QuickLook.framework/QuickLook, 2): Symbol not found: __ZN3WTF6String6formatEPKcz
  Referenced from: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebInspector.framework/WebInspector
  Expected in: /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/WebKitBuild/Debug-iphonesimulator/JavaScriptCore.framework/JavaScriptCore
 in /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebInspector.framework/WebInspector
frameworkLibrary
./platform/ios/QuickLookSoftLink.mm(32) : void *WebCore::QuickLookLibrary(bool)_block_invoke
1   0x11c4b7e89 WTFCrash
2   0x12087f6e8 invocation function for block in WebCore::QuickLookLibrary(bool)
3   0x11481a779 _dispatch_client_callout
4   0x11481bc0a dispatch_once_f
5   0x12087f635 WebCore::QuickLookLibrary(bool)
6   0x120889799 invocation function for block in WebCore::initQuickLookQLTypeCopyBestMimeTypeForFileNameAndMimeType(NSString*, NSString*)
7   0x11481a779 _dispatch_client_callout
8   0x11481bc0a dispatch_once_f
9   0x12087f7a1 WebCore::initQuickLookQLTypeCopyBestMimeTypeForFileNameAndMimeType(NSString*, NSString*)
10  0x120a31904 WebCore::softLink_QuickLook_QLTypeCopyBestMimeTypeForFileNameAndMimeType(NSString*, NSString*)
11  0x120a31308 WebCore::adjustMIMETypeIfNecessary(_CFURLResponse*, bool)
12  0x10e3c95c4 -[WKNetworkSessionDelegate URLSession:dataTask:didReceiveResponse:completionHandler:]
13  0x10d955363 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__
14  0x10d9551ca -[NSBlockOperation main]
15  0x10d9536b2 -[__NSOperationInternal _start:]
16  0x11481a779 _dispatch_client_callout
17  0x11481f931 _dispatch_block_invoke_direct
18  0x11481a779 _dispatch_client_callout
19  0x11481f931 _dispatch_block_invoke_direct
20  0x11481f7d4 dispatch_block_perform
21  0x10d94f75b __NSOQSchedule_f
22  0x11481a779 _dispatch_client_callout
23  0x114824778 _dispatch_main_queue_callback_4CF
24  0x11305bc99 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
25  0x11301fea6 __CFRunLoopRun
26  0x11301f30b CFRunLoopRunSpecific
27  0x10d93ab4a -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
28  0x10d93aa25 -[NSRunLoop(NSRunLoop) run]
29  0x114c478c9 _xpc_objc_main
30  0x114c49d73 xpc_main
31  0x10d88ed9e main
Comment 17 Dawei Fenton (:realdawei) 2018-08-02 08:45:16 PDT
Reverted r234489 for reason:

Caused 50+ crashes and 60+ API failures on iOS

Committed r234501: <https://trac.webkit.org/changeset/234501>
Comment 18 Darin Adler 2018-08-02 09:25:10 PDT
I guess there are other processes on iOS that depend on linking to the WTF::String::Format function!? We need to fix that.
Comment 19 Darin Adler 2018-08-02 09:27:11 PDT
Wait, no, it’s WebInspector.

The problem here is that when WebKit calls out to QuickLook, QuickLook loads the *system* version of the WebInspector framework, not the one that was just built.
Comment 20 Darin Adler 2018-08-02 09:28:35 PDT
Who’s an expert on how this is supposed to work in when testing builds on the iOS Simulator that can help us figure out how to fix this?
Comment 21 Jonathan Bedard 2018-08-02 09:50:53 PDT
(In reply to Darin Adler from comment #20)
> Who’s an expert on how this is supposed to work in when testing builds on
> the iOS Simulator that can help us figure out how to fix this?

This sounds like something has gone wrong with our DYLD paths. Perhaps we don't have a WebInspector framework in the build directory?  Looking into it.
Comment 22 BJ Burg 2018-08-02 10:11:09 PDT
(In reply to Darin Adler from comment #19)
> Wait, no, it’s WebInspector.
> 
> The problem here is that when WebKit calls out to QuickLook, QuickLook loads
> the *system* version of the WebInspector framework, not the one that was
> just built.

WebInspector.framework is not built from the WebKit tree, it's from Safari tree. So for open source builds, this is expected.

I couldn't find any explicit usages of String::format in the WebInspector.framework code base, but there could be implicit usages via ASSERT() or other macros that are used.
Comment 23 Jonathan Bedard 2018-08-02 11:05:58 PDT
(In reply to Brian Burg from comment #22)
> (In reply to Darin Adler from comment #19)
> > Wait, no, it’s WebInspector.
> > 
> > The problem here is that when WebKit calls out to QuickLook, QuickLook loads
> > the *system* version of the WebInspector framework, not the one that was
> > just built.
> 
> WebInspector.framework is not built from the WebKit tree, it's from Safari
> tree. So for open source builds, this is expected.
> 
> I couldn't find any explicit usages of String::format in the
> WebInspector.framework code base, but there could be implicit usages via
> ASSERT() or other macros that are used.

Maybe we need both String::format and String::deprecatedFormat for the next few months.
Comment 24 Joseph Pecoraro 2018-08-02 11:15:03 PDT
Hmm, yes there are files such as:
Source/WTF/wtf/mac/DeprecatedSymbolsUsedBySafari.mm
Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp

These files play tricks to add symbols back so that older Apple binaries (such as non-trunk iOS Simulator frameworks, or non-trunk SafariForWebKitDevelopment) can continue to find the expected symbols in trunk built WebKit frameworks.

I'm guessing the best fix would be to add a String::format to:
Source/WTF/wtf/mac/DeprecatedSymbolsUsedBySafari.mm

And include a suitable warning around its definition.

Then, when the supported iOS Simulator builds have updated appropriately we can remove the DeprecatedSymbols definition.
Comment 25 Joseph Pecoraro 2018-08-02 11:28:34 PDT
(In reply to Brian Burg from comment #22)
>
> I couldn't find any explicit usages of String::format in the
> WebInspector.framework code base, but there could be implicit usages via
> ASSERT() or other macros that are used.

I also couldn't quickly figure out where this is coming from either. WebInspector.framework does link with JavaScriptCore and use some WTF features, but we try to keep it to a minimum (mostly WTF::JSON, which does use WTF::String).
Comment 26 BJ Burg 2018-08-02 14:20:07 PDT
This symbol is not referenced by WebInspector.framework in trunk:

midnight:OpenSource bburg$ nm /Users/bburg/repos/webkit/OpenSource/WebKitBuild/Release/WebInspector.framework/WebInspector | grep ZN3WTF6String
                 U __ZN3WTF6StringC1ENS_12ASCIILiteralE
                 U __ZN3WTF6StringC1EP8NSString

It used to be referenced in the error code path, but this was simplified by Darin (heh):

diff --git a/Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py b/Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py
index a164c028f1c..2a24718ee3a 100755
--- a/Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py
+++ b/Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Copyright (c) 2014-2016 Apple Inc. All rights reserved.
+# Copyright (c) 2014-2018 Apple Inc. All rights reserved.
 # Copyright (c) 2014 University of Washington. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
@@ -177,7 +177,7 @@ class ObjCBackendDispatcherImplementationGenerator(ObjCGenerator):
 
                 if isinstance(parameter.type, EnumType):
                     lines.append('    if (!%s) {' % objc_in_param_name)
-                    lines.append('        backendDispatcher()->reportProtocolError(BackendDispatcher::InvalidParams, String::format("Parameter \'%%s\' of method \'%%s\' cannot be processed", "%s", "%s.%s"));' % (parameter.parameter_name, domain.domain_name, command.command_name))
+                    lines.append('        backendDispatcher()->reportProtocolError(BackendDispatcher::InvalidParams, ASCIILiteral { "Parameter \'%s\' of method \'%s.%s\' cannot be processed" });' % (parameter.parameter_name, domain.domain_name, command.command_name))
                     lines.append('        return;')
                     lines.append('    }')
Comment 27 Darin Adler 2019-02-25 06:42:41 PST
Not necessary now, because I removed String::format entirely.