[Cocoa] reduce unnecessary use of .mm source files in WTF, spruce up some implementation details
Created attachment 343329 [details] Patch
This is better for the ARC transition since .cpp files aren’t affected by ARC.
Comment on attachment 343329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343329&action=review > Source/WTF/wtf/cocoa/CPUTimeCocoa.cpp:83 > + return Seconds(info.user_time.seconds + info.system_time.seconds) + Seconds::fromMicroseconds(info.user_time.microseconds + info.system_time.microseconds); Could make this use uniform initialization.
(In reply to Anders Carlsson from comment #3) > Comment on attachment 343329 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343329&action=review > > > Source/WTF/wtf/cocoa/CPUTimeCocoa.cpp:83 > > + return Seconds(info.user_time.seconds + info.system_time.seconds) + Seconds::fromMicroseconds(info.user_time.microseconds + info.system_time.microseconds); > > Could make this use uniform initialization. The two arguments are int64_t but the argument is a double, so I can’t easily do that without adding type casts or local variables.
Committed r233120: <https://trac.webkit.org/changeset/233120>
<rdar://problem/41392415>
Reverted r233120 for reason: caused regression in ios API tests Committed r233186: <https://trac.webkit.org/changeset/233186>
What regression did this cause in iOS API tests?
(In reply to Darin Adler from comment #8) > What regression did this cause in iOS API tests? It caused 50+ layout tests to fail on iOS Simulator. Here's a run with crash logs in the layout test results: <https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r233120%20(5744)/results.html> This shows where the 50+ layout tests stopped crashing after this was reverted in r233186: <https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)?numbuilds=50> The thing I can't figure out is why this patch would cause these crashes. The crash is happening in QuickLookSoftLink.mm when trying to load the symbol for the QLTypeCopyBestMimeTypeForFileNameAndMimeType() function. The SOFT_LINK_FUNCTION_FOR_SOURCE() macro used in QuickLookSoftLink.mm is defined in Source/WTF/wtf/cocoa/SoftLinking.h. The only thing remotely related to that might be the inline constructor change to WTFString.h, but I'm not sure how that constructor would be used in the dispatch_once() block in the SOFT_LINK_FUNCTION_FOR_SOURCE() macro.
(In reply to David Kilzer (:ddkilzer) from comment #9) > (In reply to Darin Adler from comment #8) > > What regression did this cause in iOS API tests? > > It caused 50+ layout tests to fail on iOS Simulator. Here's a run with > crash logs in the layout test results: > <https://build.webkit.org/results/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r233120%20(5744)/ > results.html> > > This shows where the 50+ layout tests stopped crashing after this was > reverted in r233186: > <https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)?numbuilds=50> > > The thing I can't figure out is why this patch would cause these crashes. > The crash is happening in QuickLookSoftLink.mm when trying to load the > symbol for the QLTypeCopyBestMimeTypeForFileNameAndMimeType() function. The > SOFT_LINK_FUNCTION_FOR_SOURCE() macro used in QuickLookSoftLink.mm is > defined in Source/WTF/wtf/cocoa/SoftLinking.h. > > The only thing remotely related to that might be the inline constructor > change to WTFString.h, but I'm not sure how that constructor would be used > in the dispatch_once() block in the SOFT_LINK_FUNCTION_FOR_SOURCE() macro. Does this constructor need to be marked explicit? String(NSString *);
(In reply to David Kilzer (:ddkilzer) from comment #10) > Does this constructor need to be marked explicit? > > String(NSString *); No.
(In reply to David Kilzer (:ddkilzer) from comment #9) > (In reply to Darin Adler from comment #8) > > What regression did this cause in iOS API tests? > > It caused 50+ layout tests to fail on iOS Simulator. Here's a run with > crash logs in the layout test results: > <https://build.webkit.org/results/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r233120%20(5744)/ > results.html> > > This shows where the 50+ layout tests stopped crashing after this was > reverted in r233186: > <https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)?numbuilds=50> > > The thing I can't figure out is why this patch would cause these crashes. > The crash is happening in QuickLookSoftLink.mm when trying to load the > symbol for the QLTypeCopyBestMimeTypeForFileNameAndMimeType() function. The > SOFT_LINK_FUNCTION_FOR_SOURCE() macro used in QuickLookSoftLink.mm is > defined in Source/WTF/wtf/cocoa/SoftLinking.h. > > The only thing remotely related to that might be the inline constructor > change to WTFString.h, but I'm not sure how that constructor would be used > in the dispatch_once() block in the SOFT_LINK_FUNCTION_FOR_SOURCE() macro. My apologies for omitting this earlier, here's also a run with logs from the API test results: https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/5744/steps/run-api-tests/logs/stdio
(In reply to David Kilzer (:ddkilzer) from comment #9) > The crash is happening in QuickLookSoftLink.mm when trying to load the > symbol for the QLTypeCopyBestMimeTypeForFileNameAndMimeType() function. The > SOFT_LINK_FUNCTION_FOR_SOURCE() macro used in QuickLookSoftLink.mm is > defined in Source/WTF/wtf/cocoa/SoftLinking.h. I also see that in other cases the crash is when trying to load the symbol for QLPreviewGetSupportedMIMETypes, which returns an NSSet so I don’t think it has anything to do with the NSString symbols. And at least in the cases I looked at, the crash is doubly nested inside dispatch_once, which means the crash is actually in SOFT_LINK_FRAMEWORK_FOR_SOURCE in that dispatch_once block. I agree that I don’t see how the patch could have any effect on this. I don’t see it touching any file included by QuickLookSoftLink.mm. I guess I can hold off indefinitely until someone figures it out.
Just a wild guess, but I think this is a build and dependencies problem. I suspect that if we forced a full rebuild the errors would go away.
I have three ideas of how to proceed: 1) Get someone who can reproduce the simulator test crash to apply the patch, reproduce the crash, and then try a full rebuild to see if that makes the crash go away. 2) Try landing a version of this patch with everything else except for the changes to WTFString.h and StringMac.mm since it seems impossible that the other changes have any impact on the crash. 3) Set up myself so that I can to reproduce the simulator crash locally myself (may make this take a long time). Anyone want to help me decide? Your thoughts?
(In reply to Darin Adler from comment #15) > I have three ideas of how to proceed: > > 1) Get someone who can reproduce the simulator test crash to apply the > patch, reproduce the crash, and then try a full rebuild to see if that makes > the crash go away. 1a) Reland the patch and have a bot watcher force a clean build on all the bots. It’s too bad we don’t have a “force clean build” flag when marking a bug for commit queue. (If course it would be better to try to fix the dependency, but that may not be the best use of time?) > 2) Try landing a version of this patch with everything else except for the > changes to WTFString.h and StringMac.mm since it seems impossible that the > other changes have any impact on the crash. I would start with (2), then coordinae with someone to do (1a) for the second part.
Committed r233380: <https://trac.webkit.org/changeset/233380>
OK, started with (2). Landed a version of the patch without the WTFString.h change.
Reopening to attach new patch.
Created attachment 344655 [details] Part 2
(In reply to David Kilzer (:ddkilzer) from comment #20) > Created attachment 344655 [details] > Part 2 I did a clean iOS Simulator Release build with this patch, and ran the tests that failed before (on iOS 12, not iOS 11.x): $ ./Tools/Scripts/run-webkit-tests --ios-simulator --release css3 dom Using port 'ios-simulator' Test configuration: <, x86_64, release> Placing test results in /var/build/Release-iphonesimulator/layout-test-results Baseline search path: platform/ios-simulator-12-wk2 -> platform/ios-simulator-12 -> platform/ios-simulator-wk2 -> platform/ios-simulator -> platform/ios-12 -> platform/ios-wk2 -> platform/ios -> platform/wk2 -> generic Using Release build Pixel tests disabled Regular timeout: 30000, slow test timeout: 150000 Command line: /var/build/Release-iphonesimulator/WebKitTestRunnerApp.app - Found 4435 tests; running 4345, skipping 90. Running 4345 tests Running 2 WebKitTestRunnerApp.apps in parallel. [44/4345] css3/font-variant-small-caps-synthesis-coverage.html passed unexpectedly css3/color/gradients.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-backgrounds-borders.html -> ref test hashes didn't match but diff passed css3/color-filters/svg/color-filter-inline-svg.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-box-shadow.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-color-property-list-item.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-color-property.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-color-text-decorations.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-column-rule.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-composition-underline-color.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-contrast.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-current-color.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-inherits.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-opacity.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-outline.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-text-decoration-shadow.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-text-emphasis.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-text-shadow.html -> ref test hashes didn't match but diff passed css3/color-filters/color-filter-text-stroke.html -> ref test hashes didn't match but diff passed css3/filters/backdrop/backdrop-filter-with-clip-path.html is a reftest, but has an unused expectation file. Please remove /Users/ddkilzer/Data/WebKit.git/LayoutTests/css3/filters/backdrop/backdrop-filter-with-clip-path-expected.txt. [452/4345] css3/filters/backdrop/dynamic-with-clip-path.html passed unexpectedly [554/4345] css3/flexbox/image-percent-max-height.html passed unexpectedly [985/4345] css3/flexbox/csswg/ttwf-reftest-flex-wrap-reverse.html passed unexpectedly [990/4345] css3/flexbox/csswg/ttwf-reftest-flex-wrap.html passed unexpectedly 4340 tests ran as expected, 5 didn't: Expected to fail, but passed: (5) css3/filters/backdrop/dynamic-with-clip-path.html css3/flexbox/csswg/ttwf-reftest-flex-wrap-reverse.html css3/flexbox/csswg/ttwf-reftest-flex-wrap.html css3/flexbox/image-percent-max-height.html css3/font-variant-small-caps-synthesis-coverage.html
Comment on attachment 344655 [details] Part 2 Attachment 344655 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8489519 Number of test failures exceeded the failure limit.
Created attachment 344660 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 344655 [details] Part 2 Attachment 344655 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8491135 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 344675 [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
Moving back to RESOLVED/FIXED state. I'm not set up to test iOS 11.x Simulator at this time.