Bug 172797 - Make WebCore::defaultPortForProtocol() thread-safe
Summary: Make WebCore::defaultPortForProtocol() thread-safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-31 20:57 PDT by Brent Fulgham
Modified: 2017-06-01 14:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2017-05-31 21:05 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (967.69 KB, application/zip)
2017-06-01 00:09 PDT, Build Bot
no flags Details
Patch (3.84 KB, patch)
2017-06-01 12:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2017-06-01 13:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2017-06-01 13:12 PDT, Chris Dumez
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-05-31 20:57:34 PDT
Accessing WebCore::defaultPortForProtocol from multiple threads is not safe because of the static DefaultPortForProtocolMapForTesting HashMap.

Add an ASSERT to make sure we are not accessing this method from multiple threads.
Comment 1 Brent Fulgham 2017-05-31 21:05:32 PDT
Created attachment 311681 [details]
Patch
Comment 2 Build Bot 2017-06-01 00:09:54 PDT
Comment on attachment 311681 [details]
Patch

Attachment 311681 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3852645

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2017-06-01 00:09:55 PDT
Created attachment 311685 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 WebKit Commit Bot 2017-06-01 09:54:23 PDT
Comment on attachment 311681 [details]
Patch

Clearing flags on attachment: 311681

Committed r217660: <http://trac.webkit.org/changeset/217660>
Comment 5 WebKit Commit Bot 2017-06-01 09:54:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Chris Dumez 2017-06-01 10:04:16 PDT
Reverted r217660 for reason:

This should not have landed given that the new assertion is hit on debug EWS bots

Committed r217661: <http://trac.webkit.org/changeset/217661>
Comment 7 Chris Dumez 2017-06-01 12:33:47 PDT
Make WebCore::defaultPortForProtocol() thread-safe since it is called from the SecurityOrigin constructor and SecurityOrigin objects are constructed from various threads.
Comment 8 Chris Dumez 2017-06-01 12:53:34 PDT
Created attachment 311741 [details]
Patch
Comment 9 Chris Dumez 2017-06-01 13:05:41 PDT
Created attachment 311745 [details]
Patch
Comment 10 Chris Dumez 2017-06-01 13:12:47 PDT
Created attachment 311748 [details]
Patch
Comment 11 Brent Fulgham 2017-06-01 13:52:13 PDT
Comment on attachment 311748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311748&action=review

r=me

> Source/WebCore/platform/URL.cpp:547
> +static Lock& defaultPortForProtocolMapLock()

Maybe call this "defaultPortForProtocolMapForTestingLock" to match the other methods?

> Source/WebCore/platform/URL.cpp:556
> +    static DefaultPortForProtocolMapForTesting* defaultPortForProtocolMap;

Do we need to initialize this to nullptr?
Comment 12 Chris Dumez 2017-06-01 14:00:10 PDT
Comment on attachment 311748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311748&action=review

>> Source/WebCore/platform/URL.cpp:547
>> +static Lock& defaultPortForProtocolMapLock()
> 
> Maybe call this "defaultPortForProtocolMapForTestingLock" to match the other methods?

Ok

>> Source/WebCore/platform/URL.cpp:556
>> +    static DefaultPortForProtocolMapForTesting* defaultPortForProtocolMap;
> 
> Do we need to initialize this to nullptr?

This is not needed here. Statics are guaranteed to be implicitly zero-initialized by both C and C++ standards.
Comment 13 Chris Dumez 2017-06-01 14:03:03 PDT
Committed r217682: <http://trac.webkit.org/changeset/217682>