Changeset 216217 in webkit for trunk/Source/JavaScriptCore/replay


Ignore:
Timestamp:
May 4, 2017, 4:24:13 PM (8 years ago)
Author:
[email protected]
Message:

NeverDestroyed<String>(ASCIILiteral(...)) is not thread safe.
​https://bugs.webkit.org/show_bug.cgi?id=171586
<rdar://problem/31873190>

Reviewed by Yusuke Suzuki.

Source/JavaScriptCore:

JavaScriptCore allows multiple VMs to be instantiated, and each of these should
be able to run concurrently on different threads. There is code in the VM that
allocates NeverDestroyed<String>(ASCIILiteral(...)) to defined immortal strings
meant to be shared by all VMs.

However, NeverDestroyed<String>(ASCIILiteral(...)) is not thread-safe because
each thread will ref and deref the underlying StringImpl. Since this ref and
deref is not done in a thread-safe way, the NeverDestroyed<String> may get
destroyed due to the ref/deref races. Additionally, each thread may modify the
StringImpl by setting its hash and also twiddling its flags.

The fix is to use the StaticStringImpl class which is safe for ref/derefing
concurrently from different threads. StaticStringImpl is also pre-set with a
hash on construction, and its flags are set in such a way as to prevent twiddling
at runtime. Hence, we will be able to share a NeverDestroyed<String> between
VMs, as long as it is backed by a StaticStringImpl.

An alternative solution would be to change all the uses of NeverDestroyed<String>
to use per-VM strings. However, this solution is cumbersome, and makes it harder
to allocate the intended shared string. It also uses more memory and takes more
CPU time because it requires allocating the same string for each VM instance.
The StaticStringImpl solution wins out because it is more efficient and is easier
to use.

The StaticStringImpl solution also can be used in WTF without a layer violation.
See Source/WTF/wtf/text/icu/TextBreakIteratorICU.h for an example.

Also added the MultithreadedMultiVMExecutionTest which runs multiple VMs in
multiple threads, all banging on the BuiltinExecutable's baseConstructorCode
NeverDestroyed<String>. The test will manifest the issue reliably (before this
fix) if run on an ASAN build.

  • API/tests/MultithreadedMultiVMExecutionTest.cpp: Added.

(threadsList):
(startMultithreadedMultiVMExecutionTest):
(finalizeMultithreadedMultiVMExecutionTest):

  • API/tests/MultithreadedMultiVMExecutionTest.h: Added.
  • API/tests/testapi.c:

(main):

  • JavaScriptCore.xcodeproj/project.pbxproj:
  • builtins/BuiltinExecutables.cpp:

(JSC::BuiltinExecutables::createDefaultConstructor):

  • inspector/agents/InspectorDebuggerAgent.cpp:

(Inspector::objectGroupForBreakpointAction):

  • replay/scripts/CodeGeneratorReplayInputsTemplates.py:
  • replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::SavedMouseButton>::type):

  • replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::SavedMouseButton>::type):

  • replay/scripts/tests/expected/generate-enum-with-guard.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::HandleWheelEvent>::type):

  • replay/scripts/tests/expected/generate-enums-with-same-base-name.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::FormCombo>::type):

  • replay/scripts/tests/expected/generate-input-with-guard.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::GetCurrentTime>::type):
(JSC::InputTraits<Test::SetRandomSeed>::type):

  • replay/scripts/tests/expected/generate-input-with-vector-members.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::ArrayOfThings>::type):
(JSC::InputTraits<Test::SavedHistory>::type):

  • replay/scripts/tests/expected/generate-inputs-with-flags.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::ScalarInput1>::type):
(JSC::InputTraits<Test::ScalarInput2>::type):

  • replay/scripts/tests/expected/generate-memoized-type-modes.json-TestReplayInputs.cpp:

(JSC::InputTraits<Test::ScalarInput>::type):
(JSC::InputTraits<Test::MapInput>::type):

  • runtime/IntlObject.cpp:

(JSC::numberingSystemsForLocale):

Source/WebCore:

No new tests because we're just converting uses of ASCIILiteral (in the
instantiation of NeverDestroyed<String> and NeverDestroyed<const String>) to
MAKE_STATIC_STRING_IMPL.

The correctness of using MAKE_STATIC_STRING_IMPL is tested in the newly added
API test in this patch.

Also changed "static NeverDestroyed<ASCIILiteral>" instances in
SQLiteIDBBackingStore.cpp to "static const char* const" because they are only
ever used to get the underlying const char*.

  • Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:

(WebCore::IDBServer::SQLiteIDBBackingStore::getRecord):
(WebCore::IDBServer::SQLiteIDBBackingStore::cachedStatementForGetAllObjectStoreRecords):

  • Modules/mediastream/MediaEndpointSessionDescription.cpp:
  • Modules/mediastream/RTCRtpTransceiver.cpp:
  • Modules/mediastream/SDPProcessor.cpp:
  • Modules/navigatorcontentutils/NavigatorContentUtils.cpp:

(WebCore::customHandlersStateString):
(WebCore::NavigatorContentUtils::isProtocolHandlerRegistered):

  • Modules/speech/SpeechSynthesis.cpp:

(WebCore::SpeechSynthesis::boundaryEventOccurred):

  • accessibility/AccessibilityMediaControls.cpp:

(WebCore::AccessibilityMediaControl::controlTypeName):
(WebCore::AccessibilityMediaControl::title):
(WebCore::AccessibilityMediaControlsContainer::elementTypeName):
(WebCore::AccessibilityMediaTimeline::helpText):
(WebCore::AccessibilityMediaTimeDisplay::accessibilityDescription):

  • bindings/js/JSLazyEventListener.cpp:

(WebCore::eventParameterName):

  • contentextensions/ContentExtensionsBackend.cpp:

(WebCore::ContentExtensions::ContentExtensionsBackend::displayNoneCSSRule):

  • css/CSSDefaultStyleSheets.cpp:

(WebCore::screenEval):
(WebCore::printEval):

  • css/MediaList.cpp:

(WebCore::addResolutionWarningMessageToConsole):

  • css/StyleSheetContents.cpp:

(WebCore::StyleSheetContents::parseAuthorStyleSheet):

  • dom/Document.cpp:

(WebCore::Document::readyState):

  • dom/LoadableClassicScript.cpp:

(WebCore::LoadableClassicScript::notifyFinished):

  • dom/PseudoElement.cpp:

(WebCore::PseudoElement::pseudoElementNameForEvents):

  • editing/MarkupAccumulator.cpp:

(WebCore::MarkupAccumulator::shouldAddNamespaceElement):

  • editing/cocoa/DataDetection.mm:

(WebCore::DataDetection::dataDetectorURLProtocol):

  • editing/markup.cpp:

(WebCore::StyledMarkupAccumulator::styleNodeCloseTag):
(WebCore::createMarkupInternal):

  • html/FormController.cpp:

(WebCore::formStateSignature):

  • html/ImageInputType.cpp:

(WebCore::ImageInputType::appendFormData):

  • html/canvas/CanvasRenderingContext2D.cpp:

(WebCore::CanvasRenderingContext2D::realizeSaves):
(WebCore::CanvasRenderingContext2D::getImageData):

  • html/parser/XSSAuditor.cpp:

(WebCore::XSSAuditor::init):
(WebCore::XSSAuditor::eraseDangerousAttributesIfInjected):

  • html/track/VTTCue.cpp:

(WebCore::startKeyword):
(WebCore::middleKeyword):
(WebCore::endKeyword):
(WebCore::leftKeyword):
(WebCore::rightKeyword):
(WebCore::verticalGrowingLeftKeyword):
(WebCore::verticalGrowingRightKeyword):
(WebCore::VTTCue::determineTextDirection):
(WebCore::VTTCue::markFutureAndPastNodes):

  • inspector/InspectorCSSAgent.cpp:

(WebCore::computePseudoClassMask):

  • inspector/InspectorIndexedDBAgent.cpp:
  • inspector/InspectorPageAgent.cpp:

(WebCore::InspectorPageAgent::sourceMapURLForResource):

  • inspector/PageDebuggerAgent.cpp:

(WebCore::PageDebuggerAgent::sourceMapURLForScript):

  • loader/ImageLoader.cpp:

(WebCore::ImageLoader::notifyFinished):

  • loader/TextTrackLoader.cpp:

(WebCore::TextTrackLoader::corsPolicyPreventedLoad):

  • loader/icon/IconDatabase.cpp:

(WebCore::IconDatabase::defaultDatabaseFilename):

  • page/CaptionUserPreferencesMediaAF.cpp:

(WebCore::CaptionUserPreferencesMediaAF::captionsTextEdgeCSS):

  • page/SecurityOrigin.cpp:

(WebCore::SecurityOrigin::urlWithUniqueSecurityOrigin):

  • page/UserContentURLPattern.cpp:

(WebCore::UserContentURLPattern::parse):

  • platform/MIMETypeRegistry.cpp:

(WebCore::defaultMIMEType):

  • platform/animation/Animation.cpp:

(WebCore::Animation::initialName):

  • platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:

(WebCore::MediaPlayerPrivateMediaSourceAVFObjC::engineDescription):

  • platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:

(WebCore::MediaPlayerPrivateMediaStreamAVFObjC::engineDescription):

  • platform/graphics/cocoa/FontCacheCoreText.cpp:

(WebCore::FontCache::similarFont):

  • platform/gtk/UserAgentGtk.cpp:

(WebCore::platformVersionForUAString):

  • platform/mock/mediasource/MockBox.cpp:

(WebCore::MockTrackBox::type):
(WebCore::MockInitializationBox::type):
(WebCore::MockSampleBox::type):

  • platform/network/HTTPHeaderValues.cpp:

(WebCore::HTTPHeaderValues::textPlainContentType):
(WebCore::HTTPHeaderValues::formURLEncodedContentType):
(WebCore::HTTPHeaderValues::noCache):
(WebCore::HTTPHeaderValues::maxAge0):

  • platform/network/HTTPParsers.cpp:

(WebCore::parseXSSProtectionHeader):

  • replay/MemoizedDOMResult.cpp:

(JSC::InputTraits<MemoizedDOMResultBase>::type):

  • svg/SVGTransformValue.cpp:

(WebCore::SVGTransformValue::transformTypePrefixForParsing):

Source/WebKit2:

  • Shared/API/APIError.cpp:

(API::Error::webKitErrorDomain):
(API::Error::webKitNetworkErrorDomain):
(API::Error::webKitPolicyErrorDomain):
(API::Error::webKitPluginErrorDomain):
(API::Error::webKitDownloadErrorDomain):
(API::Error::webKitPrintErrorDomain):

  • Shared/WebPreferencesKeys.cpp:
  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::executeEditCommand):

  • WebProcess/WebCoreSupport/WebEditorClient.cpp:

(WebKit::WebEditorClient::didBeginEditing):
(WebKit::WebEditorClient::respondToChangedContents):
(WebKit::WebEditorClient::respondToChangedSelection):
(WebKit::WebEditorClient::didEndEditing):

Source/WTF:

StaticStringImpl is meant to be thread-safe. However, it has a bug: it did not
set the s_hashFlagDidReportCost flag. As a result, if cost() is called on it,
different threads may try to change its flags bits at the same time. This patch
changes StaticStringImpl to always set the s_hashFlagDidReportCost flag.

Also factored out StringImplShape and made StringImpl and StaticStringImpl extend
it. This makes it more clear that the 2 are intended to have the same shape.
Note: there is already a static_assert that the 2 have the same size. This
change also ensures that they both have the same shape, which is a requirement in
order for StaticStringImpl to work.

Introduced the MAKE_STATIC_STRING_IMPL macro as a convenient way to instantiate
StaticStringImpls from literal strings. This allows us to trivially change

NeverDestroyed<String> myString(ASCIILiteral("myString"));

to ...

NeverDestroyed<String> myString(MAKE_STATIC_STRING_IMPL("myString"));

and by so doing, make it thread-safe.

MAKE_STATIC_STRING_IMPL instantiates a lambda function to create the static
StaticStringImpls.

  • wtf/text/StringImpl.h:

(WTF::StringImplShape::StringImplShape):
(WTF::StringImpl::StringImpl):
(WTF::StringImpl::cost):
(WTF::StringImpl::setHash):
(WTF::StringImpl::StaticStringImpl::StaticStringImpl):
(WTF::StringImpl::StaticStringImpl::operator StringImpl&):

  • wtf/text/WTFString.h:

(WTF::String::String):

  • wtf/text/icu/TextBreakIteratorICU.h:

(WTF::caretRules):

Tools:

API test for exercising StaticStringImpl and the MAKE_STATIC_STRING_IMPL macro.

  • TestWebKitAPI/Tests/WTF/StringImpl.cpp:

(TestWebKitAPI::neverDestroyedString):
(TestWebKitAPI::getNeverDestroyedStringAtStackDepth):
(TestWebKitAPI::TEST):

Location:
trunk/Source/JavaScriptCore/replay/scripts
Files:
9 edited

Legend:

Unmodified
Added
Removed
Note: See TracChangeset for help on using the changeset viewer.