Bug 214102 – JSRunLoopTimer should use WTF::RunLoop rather than custom CF code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

JSRunLoopTimer should use WTF::RunLoop rather than custom CF code

[Geoffrey Garen]

Created attachment 403813 [details]
Patch

[Geoffrey Garen]

Created attachment 403879 [details]
Patch

[Geoffrey Garen]

We have testing for this now in https://bugs.webkit.org/show_bug.cgi?id=214193.

[Darin Adler]

Comment on attachment 403879 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:73
> +            PerVMData(Manager&, RefPtr<WTF::RunLoop>);

Seems like this argument should be WTF::RunLoop&.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:76
> +            RefPtr<WTF::RunLoop> runLoop;

Can this be Ref instead of RefPtr?

> Source/JavaScriptCore/runtime/VM.h:357
> +    RefPtr<WTF::RunLoop> m_runLoop;

Can this be Ref instead of RefPtr?

> Source/JavaScriptCore/runtime/VM.h:1080
> +    WTF::RunLoop* runLoop() const { return m_runLoop.get(); }

Can this return a reference rather than a pointer? It can never be null, right?

[Geoffrey Garen]

*** Bug 214193 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

Created attachment 403997 [details]
Patch

[Darin Adler]

Comment on attachment 403997 [details]
Patch

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

> Source/JavaScriptCore/runtime/VM.h:1080
> +    WTF::RunLoop& runLoop() const { return m_runLoop.get(); }

Don’t actually need get() here.

> Source/WebCore/bindings/js/CommonVM.cpp:61
> +    RunLoop* runLoop = &RunLoop::current();

Seems like this could just be nullptr since current is the default.

[Geoffrey Garen]

Created attachment 404000 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

[EWS]

Committed r264242: <https://trac.webkit.org/changeset/264242>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404000 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65366297>

[Chris Dumez]

Reverted r264242 for reason:

Caused many crashes on iOS bots

Committed r264262: <https://trac.webkit.org/changeset/264262>

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> Reverted r264242 for reason:
> 
> Caused many crashes on iOS bots
> 
> Committed r264262: <https://trac.webkit.org/changeset/264262>

https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20(Tests)/r264243%20(4626)/results.html

ASSERTION FAILED: s_webRunLoop
/Volumes/Data/slave/ios-simulator-13-debug/build/Source/WTF/wtf/RunLoop.cpp(82) : static WTF::RunLoop &WTF::RunLoop::web()
1   0x6c5525c49 WTFCrash
2   0x6c66c77db WTFCrashWithInfo(int, char const*, char const*, int)
3   0x6c55c9d4d WTF::RunLoop::web()
4   0x6cce5c048 WebCore::commonVMSlow()
5   0x6cb204fa3 WebCore::commonVM()
6   0x6cddc8724 WebCore::PageScriptDebugServer::PageScriptDebugServer(WebCore::Page&)
7   0x6cddc877d WebCore::PageScriptDebugServer::PageScriptDebugServer(WebCore::Page&)
8   0x6cdd8adf4 WebCore::InspectorController::InspectorController(WebCore::Page&, WebCore::InspectorClient*)
9   0x6cdd8b3d5 WebCore::InspectorController::InspectorController(WebCore::Page&, WebCore::InspectorClient*)
10  0x6ce33ce32 std::__1::__unique_if<WebCore::InspectorController>::__unique_single std::__1::make_unique<WebCore::InspectorController, WebCore::Page&, WebCore::InspectorClient*&>(WebCore::Page&, WebCore::InspectorClient*&)
11  0x6ce323bb4 decltype(auto) WTF::makeUnique<WebCore::InspectorController, WebCore::Page&, WebCore::InspectorClient*&>(WebCore::Page&, WebCore::InspectorClient*&)
12  0x6ce322836 WebCore::Page::Page(WebCore::PageConfiguration&&)
13  0x6ce32554d WebCore::Page::Page(WebCore::PageConfiguration&&)
14  0x6b194c88d std::__1::__unique_if<WebCore::Page>::__unique_single std::__1::make_unique<WebCore::Page, WebCore::PageConfiguration>(WebCore::PageConfiguration&&)
15  0x6b192351f decltype(auto) WTF::makeUnique<WebCore::Page, WebCore::PageConfiguration>(WebCore::PageConfiguration&&)
16  0x6b1920474 WebKit::WebPage::WebPage(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)
17  0x6b191eb75 WebKit::WebPage::WebPage(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)
18  0x6b191ea81 WebKit::WebPage::create(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)
19  0x6b144bf84 WebKit::WebProcess::createWebPage(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)
20  0x6b1c1787c void IPC::callMemberFunctionImpl<WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>, 0ul, 1ul>(WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
21  0x6b1c162e0 void IPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>&&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&))
22  0x6b1c0fe0e void IPC::handleMessage<Messages::WebProcess::CreateWebPage, WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)>(IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&))
23  0x6b1c0de0c WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&)
24  0x6b144c996 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
25  0x6b011b89f IPC::Connection::dispatchMessage(IPC::Decoder&)
26  0x6b011c1d0 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
27  0x6b011c860 IPC::Connection::dispatchOneIncomingMessage()
28  0x6b013b618 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7::operator()()
29  0x6b013b52e WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call()
30  0x6c5551a42 WTF::Function<void ()>::operator()() const
31  0x6c55c9f55 WTF::RunLoop::performWork()

[Geoffrey Garen]

Created attachment 404045 [details]
Patch for landing - fixed iOS WK2 crash

[Geoffrey Garen]

Created attachment 404055 [details]
Patch for landing - fixed iOS WK2 crash

[Darin Adler]

Comment on attachment 404055 [details]
Patch for landing - fixed iOS WK2 crash

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

> Source/WebCore/bindings/js/CommonVM.cpp:61
> +    if (RunLoop* web = RunLoop::webIfExists())
> +        runLoop = web;

Is it OK that an early-created VM will permanently be on the current run loop, not the web run loop?

[Darin Adler]

Comment on attachment 404055 [details]
Patch for landing - fixed iOS WK2 crash

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

> Source/WebCore/bindings/js/CommonVM.cpp:60
> +    if (RunLoop* web = RunLoop::webIfExists())

No need for this if statement. Could just be:

    runLoop = RunLoop::webIfExists();

Almost makes me wish RunLoop::webIfExists was a platform-independent function that returns nullptr on non-web-thread platforms.

[Geoffrey Garen]

> Is it OK that an early-created VM will permanently be on the current run
> loop, not the web run loop?

It's a bit of a brittle design, but I believe that was the previous behavior of "vm.setRunLoop(WebThreadRunLoop())".

[Geoffrey Garen]

I wonder if there's any way to get a crash log from the api-ios bot. 🤔

[Aakash Jain]

(In reply to Geoffrey Garen from comment #19)
> I wonder if there's any way to get a crash log from the api-ios bot. 🤔
I emailed you the crash logs. There isn't any automated way currently. I or anyone from our team can manually get the logs from the bots.

[Geoffrey Garen]

Created attachment 404163 [details]
Patch for landing - fixed iOS WK2 crash

[EWS]

Committed r264315: <https://trac.webkit.org/changeset/264315>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404163 [details].

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 404897 [details]
Patch for landing - fixed WASM crash

[EWS]

Committed r264696: <https://trac.webkit.org/changeset/264696>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404897 [details].