Bug 201597

Summary: Harden JSC against the abuse of runtime options.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, cgarcia, ews-watchlist, fpizlo, gyuyoung.kim, Hironori.Fujii, keith_miller, msaboff, rakuco, rmorisset, ryuan.choi, saam, sergio, thorton, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202021
https://bugs.webkit.org/show_bug.cgi?id=209236
Attachments:
Description Flags
work in progress for EWS testing.
none
work in progress for EWS testing.
none
work in progress for EWS testing.
none
work in progress for EWS testing.
none
work in progress for EWS testing.
ews-watchlist: commit-queue-
Archive of layout-test-results from ews215 for win-future
none
patch for review.
ews-watchlist: commit-queue-
Archive of layout-test-results from ews213 for win-future
none
proposed patch.
none
proposed patch.
ews-watchlist: commit-queue-
Archive of layout-test-results from ews210 for win-future
none
Patch for GTK and WPE
none
proposed patch.
none
proposed patch.
ews-watchlist: commit-queue-
Archive of layout-test-results from ews210 for win-future
none
Patch for Windows
none
proposed patch. fpizlo: review+

Description Mark Lam 2019-09-08 18:05:47 PDT
Details to follow.
Comment 1 Radar WebKit Bug Importer 2019-09-08 18:06:24 PDT
<rdar://problem/55167068>
Comment 2 Mark Lam 2019-09-08 18:13:42 PDT
Created attachment 378342 [details]
work in progress for EWS testing.
Comment 3 Mark Lam 2019-09-09 19:59:32 PDT
Created attachment 378434 [details]
work in progress for EWS testing.
Comment 4 Mark Lam 2019-09-09 20:13:44 PDT
Created attachment 378435 [details]
work in progress for EWS testing.
Comment 5 Mark Lam 2019-09-10 12:48:03 PDT
Created attachment 378477 [details]
work in progress for EWS testing.
Comment 6 Mark Lam 2019-09-10 15:20:32 PDT
Created attachment 378499 [details]
work in progress for EWS testing.
Comment 7 EWS Watchlist 2019-09-10 17:47:26 PDT
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.
Comment 8 EWS Watchlist 2019-09-10 17:47:39 PDT
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
Comment 9 Mark Lam 2019-09-10 20:44:12 PDT
Created attachment 378534 [details]
patch for review.
Comment 10 Tim Horton 2019-09-10 21:35:09 PDT
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?
Comment 11 Mark Lam 2019-09-10 22:09:20 PDT
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.
Comment 12 Mark Lam 2019-09-10 22:13:01 PDT
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.
Comment 13 EWS Watchlist 2019-09-10 22:33:20 PDT
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.
Comment 14 EWS Watchlist 2019-09-10 22:33:22 PDT
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
Comment 15 Mark Lam 2019-09-10 22:47:32 PDT
Created attachment 378541 [details]
proposed patch.
Comment 16 Mark Lam 2019-09-10 23:39:50 PDT
Created attachment 378542 [details]
proposed patch.
Comment 17 EWS Watchlist 2019-09-11 01:32:35 PDT
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.
Comment 18 EWS Watchlist 2019-09-11 01:32:38 PDT
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
Comment 19 Carlos Garcia Campos 2019-09-11 05:28:18 PDT
Created attachment 378556 [details]
Patch for GTK and WPE

I think this is enough
Comment 20 Mark Lam 2019-09-11 07:33:25 PDT
(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.
Comment 21 Mark Lam 2019-09-11 16:32:48 PDT
Created attachment 378595 [details]
proposed patch.
Comment 22 Tim Horton 2019-09-11 17:21:27 PDT
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)
Comment 23 Mark Lam 2019-09-11 17:24:24 PDT
(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.
Comment 24 Mark Lam 2019-09-11 17:34:02 PDT
Created attachment 378604 [details]
proposed patch.
Comment 25 EWS Watchlist 2019-09-11 20:19:14 PDT
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.
Comment 26 EWS Watchlist 2019-09-11 20:19:17 PDT
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
Comment 27 Fujii Hironori 2019-09-11 21:32:07 PDT
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++
Comment 28 Mark Lam 2019-09-11 23:17:57 PDT
Created attachment 378625 [details]
proposed patch.
Comment 29 Mark Lam 2019-09-12 08:05:26 PDT
Thanks for the review.  Landed in r249808: <http://trac.webkit.org/r249808>.