Bug 186924 – [Cocoa] reduce unnecessary use of .mm source files in WTF, spruce up some implementation details

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Darin Adler]

[Cocoa] reduce unnecessary use of .mm source files in WTF, spruce up some implementation details

[Darin Adler]

Created attachment 343329 [details]
Patch

[Darin Adler]

This is better for the ARC transition since .cpp files aren’t affected by ARC.

[Anders Carlsson]

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.

[Darin Adler]

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

[Darin Adler]

Committed r233120: <https://trac.webkit.org/changeset/233120>

[Radar WebKit Bug Importer]

<rdar://problem/41392415>

[Dawei Fenton (:realdawei)]

Reverted r233120 for reason:

caused regression in ios API tests

Committed r233186: <https://trac.webkit.org/changeset/233186>

[Darin Adler]

What regression did this cause in iOS API tests?

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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 *);

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> Does this constructor need to be marked explicit?
> 
>     String(NSString *);

No.

[Dawei Fenton (:realdawei)]

(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

[Darin Adler]

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

[Darin Adler]

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.

[Darin Adler]

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?

[David Kilzer (:ddkilzer)]

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

[Darin Adler]

Committed r233380: <https://trac.webkit.org/changeset/233380>

[Darin Adler]

OK, started with (2). Landed a version of the patch without the WTFString.h change.

[David Kilzer (:ddkilzer)]

Reopening to attach new patch.

[David Kilzer (:ddkilzer)]

Created attachment 344655 [details]
Part 2

[David Kilzer (:ddkilzer)]

(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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[David Kilzer (:ddkilzer)]

Moving back to RESOLVED/FIXED state.  I'm not set up to test iOS 11.x Simulator at this time.