Bug 188191 – [WTF] Rename String::format to String::deprecatedFormat

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Template Framework

[Tomas Popela]

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.

[Tomas Popela]

Created attachment 346150 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

Do we understand what happened with the iOS simulator tests?

[Darin Adler]

(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.

[Tomas Popela]

Created attachment 346269 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 346269 [details]
Patch

Clearing flags on attachment: 346269

Committed r234489: <https://trac.webkit.org/changeset/234489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42840464>

[Dawei Fenton (:realdawei)]

(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

[Dawei Fenton (:realdawei)]

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

[Dawei Fenton (:realdawei)]

Reverted r234489 for reason:

Caused 50+ crashes and 60+ API failures on iOS

Committed r234501: <https://trac.webkit.org/changeset/234501>

[Darin Adler]

I guess there are other processes on iOS that depend on linking to the WTF::String::Format function!? We need to fix that.

[Darin Adler]

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.

[Darin Adler]

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?

[Jonathan Bedard]

(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.

[Blaze Burg]

(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.

[Jonathan Bedard]

(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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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).

[Blaze Burg]

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('    }')

[Darin Adler]

Not necessary now, because I removed String::format entirely.