Bug 201597 – Harden JSC against the abuse of runtime options.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Details to follow.

[Radar WebKit Bug Importer]

<rdar://problem/55167068>

[Mark Lam]

Created attachment 378342 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 378434 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 378435 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 378477 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 378499 [details]
work in progress for EWS testing.

[EWS Watchlist]

Comment on attachment 378499 [details]
work in progress for EWS testing.

Attachment 378499 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13021231

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 378517 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Mark Lam]

Created attachment 378534 [details]
patch for review.

[Tim Horton]

Comment on attachment 378534 [details]
patch for review.

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

The parts above JSC look fine.

> Source/WebKit/ChangeLog:11
> +        2. Removed the call enable Options::useBigInt in WebInspectorUI.
> +           WebInspectorUI doesn't really need it for now.

I assume inspector people are OK with this?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:621
> +    JSC::Config::configureForTesting();

What's the trick here? This is a WK2 test, and this is the UI process, so why do you need this/does this have any effect?

[Mark Lam]

Comment on attachment 378534 [details]
patch for review.

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

>> Source/WebKit/ChangeLog:11
>> +           WebInspectorUI doesn't really need it for now.
> 
> I assume inspector people are OK with this?

Yes, this was suggested by the Inspector folks.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:621
>> +    JSC::Config::configureForTesting();
> 
> What's the trick here? This is a WK2 test, and this is the UI process, so why do you need this/does this have any effect?

Are you sure this one is WK2?  This test was crashing without this change.  I didn't dig too deeply.  Adding this here made the crash go away.

[Mark Lam]

Comment on attachment 378534 [details]
patch for review.

I must have broken something after I rebased and added some finishing touches.  Taking this out of review while I investigate and fix.

[EWS Watchlist]

Comment on attachment 378534 [details]
patch for review.

Attachment 378534 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13022038

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 378540 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Mark Lam]

Created attachment 378541 [details]
proposed patch.

[Mark Lam]

Created attachment 378542 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 378542 [details]
proposed patch.

Attachment 378542 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13022381

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 378545 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Carlos Garcia Campos]

Created attachment 378556 [details]
Patch for GTK and WPE

I think this is enough

[Mark Lam]

(In reply to Carlos Garcia Campos from comment #19)
> Created attachment 378556 [details]
> Patch for GTK and WPE
> 
> I think this is enough

Thanks.  I'll apply the patch.

[Mark Lam]

Created attachment 378595 [details]
proposed patch.

[Tim Horton]

Comment on attachment 378595 [details]
proposed patch.

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

> Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:34
> +@property (nonatomic) BOOL configureJSCForTesting;

This needs availability annotations (also it's a bit odd to add it at the top)

[Mark Lam]

(In reply to Tim Horton from comment #22)
> Comment on attachment 378595 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378595&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:34
> > +@property (nonatomic) BOOL configureJSCForTesting;
> 
> This needs availability annotations (also it's a bit odd to add it at the
> top)

Thanks.  Will add availability annotations.  I can also move it elsewhere.  I only put there because I modeled it after injectedBundleURL right next to it.

[Mark Lam]

Created attachment 378604 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 378604 [details]
proposed patch.

Attachment 378604 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13024598

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 378614 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Fujii Hironori]

Created attachment 378619 [details]
Patch for Windows

In AppleWin, RELEASE_ASSERT is failing in JSC::JSGlobalObject::exposeDollarVM because
DumpRenderTree doesn't call JSC::Config::configureForTesting().

> JavaScriptCore.dll!abort() Line 77	C++
> JavaScriptCore.dll!JSC::JSGlobalObject::exposeDollarVM(JSC::VM & vm) Line 1829	C++
> DumpRenderTreeLib.dll!WebCoreTestSupport::injectInternalsObject(const OpaqueJSContext * context) Line 69	C++
> [Inline Frame] DumpRenderTreeLib.dll!FrameLoadDelegate::didClearWindowObjectForFrameInStandardWorld(IWebFrame *) Line 370	C++
> DumpRenderTreeLib.dll!FrameLoadDelegate::didClearWindowObjectForFrameInScriptWorld(IWebView * webView, IWebFrame * frame, IWebScriptWorld * world) Line 319	C++
> WebKit.dll!WebFrameLoaderClient::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld & world) Line 1250	C++
> WebKit.dll!WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld & world) Line 3943	C++
> WebKit.dll!WebCore::ScriptController::initScriptForWindowProxy(WebCore::JSWindowProxy & windowProxy) Line 262	C++
> WebKit.dll!WebCore::WindowProxy::createJSWindowProxyWithInitializedScript(WebCore::DOMWrapperWorld & world) Line 125	C++
> [Inline Frame] WebKit.dll!WebCore::WindowProxy::jsWindowProxy(WebCore::DOMWrapperWorld &) Line 70	C++
> [Inline Frame] WebKit.dll!WebCore::ScriptController::jsWindowProxy(WebCore::DOMWrapperWorld &) Line 337	C++
> WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld & world, WebCore::ExceptionDetails * exceptionDetails) Line 123	C++
> WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode, WebCore::ExceptionDetails * exceptionDetails) Line 149	C++
> WebKit.dll!WebCore::ScriptElement::executeClassicScript(const WebCore::ScriptSourceCode & sourceCode) Line 390	C++
> WebKit.dll!WebCore::ScriptElement::prepareScript(const WTF::TextPosition & scriptStartPosition, WebCore::ScriptElement::LegacyTypeSupport supportLegacyTypes) Line 267	C++
> WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement & scriptElement, const WTF::TextPosition & scriptStartPosition) Line 252	C++
> WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement,WTF::DumbPtrTraits<WebCore::ScriptElement> > && element, const WTF::TextPosition & scriptStartPosition) Line 142	C++
> WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() Line 234	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode mode, bool parsingFragment, WebCore::PumpSession & session) Line 255	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 309	C++
> [Inline Frame] WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) Line 186	C++
> WebKit.dll!WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl,WTF::DumbPtrTraits<WTF::StringImpl> > && inputSource) Line 419	C++
> WebKit.dll!WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter & writer, const char * data, unsigned int length) Line 50	C++
> WebKit.dll!WebCore::DocumentWriter::addData(const char * bytes, unsigned int length) Line 246	C++
> WebKit.dll!WebCore::DocumentLoader::commitData(const char * bytes, unsigned int length) Line 1161	C++
> WebKit.dll!WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader * loader, const char * data, int length) Line 695	C++
> WebKit.dll!WebCore::DocumentLoader::commitLoad(const char * data, int length) Line 1049	C++
> WebKit.dll!WebCore::DocumentLoader::dataReceived(const char * data, int length) Line 1194	C++
> WebKit.dll!WebCore::DocumentLoader::dataReceived(WebCore::CachedResource & resource, const char * data, int length) Line 1167	C++
> WebKit.dll!WebCore::CachedRawResource::notifyClientsDataWasReceived(const char * data, unsigned int length) Line 135	C++
> WebKit.dll!WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer & data) Line 74	C++
> WebKit.dll!WebCore::SubresourceLoader::didReceiveDataOrBuffer(const char * data, int length, WTF::RefPtr<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 482	C++
> WebKit.dll!WebCore::SubresourceLoader::didReceiveBuffer(WTF::Ref<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 461	C++
> WebKit.dll!WebCore::ResourceLoader::didReceiveBuffer(WebCore::ResourceHandle * __formal, WTF::Ref<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, int encodedDataLength) Line 694	C++
> [Inline Frame] WebKit.dll!WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData::__l2::<lambda_5f349daa7f72b184051aee54a6289cd4>::operator()() Line 225	C++
> WebKit.dll!WTF::Detail::CallableWrapper<<lambda_5f349daa7f72b184051aee54a6289cd4>,void>::call() Line 52	C++
> [Inline Frame] WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 79	C++
> WTF.dll!WTF::dispatchFunctionsFromMainThread() Line 117	C++
> WTF.dll!WTF::ThreadingWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam) Line 51	C++
> [External Code]	
> [Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
> DumpRenderTreeLib.dll!runTest(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & inputLine) Line 1264	C++

[Mark Lam]

Created attachment 378625 [details]
proposed patch.

[Mark Lam]

Thanks for the review.  Landed in r249808: <http://trac.webkit.org/r249808>.