Bug 172797

Summary: Make WebCore::defaultPortForProtocol() thread-safe
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, buildbot, cdumez, commit-queue, dbates, ggaren, wilander
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=172786
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Patch
none
Patch
none
Patch bfulgham: review+

Brent Fulgham
Reported 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.
Attachments
Patch (1.54 KB, patch)
2017-05-31 21:05 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (3.84 KB, patch)
2017-06-01 13:05 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (3.84 KB, patch)
2017-06-01 13:12 PDT, Chris Dumez 		bfulgham: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Brent Fulgham
Comment 1 2017-05-31 21:05:32 PDT
Created attachment 311681 [details] Patch
Build Bot
Comment 2 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.
Build Bot
Comment 3 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
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2017-06-01 09:54:25 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 6 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>
Chris Dumez
Comment 7 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.
Chris Dumez
Comment 8 2017-06-01 12:53:34 PDT
Created attachment 311741 [details] Patch
Chris Dumez
Comment 9 2017-06-01 13:05:41 PDT
Created attachment 311745 [details] Patch
Chris Dumez
Comment 10 2017-06-01 13:12:47 PDT
Created attachment 311748 [details] Patch
Brent Fulgham
Comment 11 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?
Chris Dumez
Comment 12 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.
Chris Dumez
Comment 13 2017-06-01 14:03:03 PDT
Committed r217682: <http://trac.webkit.org/changeset/217682>
Note You need to log in before you can comment on or make changes to this bug.