There are multiple places where NSApp is used in WebCore. Since we have switched to using NSRunLoop instead of the NSApplication run loop for the WebContent process, we should assert that NSApp is not used in the WebContent process.
Created attachment 333231 [details] Patch
Created attachment 333252 [details] Patch
Created attachment 333253 [details] Patch
Created attachment 333290 [details] Patch
<rdar://problem/37316144>
Created attachment 333295 [details] Patch
Comment on attachment 333295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333295&action=review > Source/WebCore/ChangeLog:14 > + In WebCore, there are a few places where NSApp is referenced. Since the WebContent process > + is no longer using the NSApplication run loop, and NSApp is no longer guaranteed to be > + valid, we should make sure that the NSApp is not referenced by the WebContent process or > + the Network process, by asserting that the NSApplication event loop is running when NSApp > + is referenced. It is still ok for the UIProcess to reference NSApp. Adding these assert > + will help catch references to NSApp when the NSApplication run loop is not used. It's not clear to me that all these places are code that never runs in the WebProcess. Is that true? Are these using NSApp because they are for WK1?
(In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 333295 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333295&action=review > > > Source/WebCore/ChangeLog:14 > > + In WebCore, there are a few places where NSApp is referenced. Since the WebContent process > > + is no longer using the NSApplication run loop, and NSApp is no longer guaranteed to be > > + valid, we should make sure that the NSApp is not referenced by the WebContent process or > > + the Network process, by asserting that the NSApplication event loop is running when NSApp > > + is referenced. It is still ok for the UIProcess to reference NSApp. Adding these assert > > + will help catch references to NSApp when the NSApplication run loop is not used. > > It's not clear to me that all these places are code that never runs in the > WebProcess. Is that true? Are these using NSApp because they are for WK1? Yes, I believe these are for WK1. EventHandlerMac.mm: * lastEventIsMouseUp() is only called by WK1, early return in EventHandler::passMouseDownEventToWidget for WK2. * sendFakeEventsAfterWidgetTracking() is only called by PoupMenuMac.mm in WK1. WebVideoFullscreenController.mm: * only used by WebView.mm in WK1. WebWindowAnimation.mm: * only used by WebVideoFullscreenController.mm (previous comment). PasteboardMac.mm: * I am not quite sure about this one. I will need to dig further. Thanks for reviewing, Simon!
(In reply to Per Arne Vollan from comment #8) > (In reply to Simon Fraser (smfr) from comment #7) > > Comment on attachment 333295 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=333295&action=review > > > > > Source/WebCore/ChangeLog:14 > > > + In WebCore, there are a few places where NSApp is referenced. Since the WebContent process > > > + is no longer using the NSApplication run loop, and NSApp is no longer guaranteed to be > > > + valid, we should make sure that the NSApp is not referenced by the WebContent process or > > > + the Network process, by asserting that the NSApplication event loop is running when NSApp > > > + is referenced. It is still ok for the UIProcess to reference NSApp. Adding these assert > > > + will help catch references to NSApp when the NSApplication run loop is not used. > > > > It's not clear to me that all these places are code that never runs in the > > WebProcess. Is that true? Are these using NSApp because they are for WK1? > > Yes, I believe these are for WK1. > > EventHandlerMac.mm: > * lastEventIsMouseUp() is only called by WK1, early return in > EventHandler::passMouseDownEventToWidget for WK2. > * sendFakeEventsAfterWidgetTracking() is only called by PoupMenuMac.mm in > WK1. > > WebVideoFullscreenController.mm: > * only used by WebView.mm in WK1. > > WebWindowAnimation.mm: > * only used by WebVideoFullscreenController.mm (previous comment). > > PasteboardMac.mm: > * I am not quite sure about this one. I will need to dig further. > > Thanks for reviewing, Simon! It seems the Pasteboard::setDragImage can be called in the WebProcess as well, but its call to [NSApp postEvent] was added a long time ago, and is not relevant in WK2.
Created attachment 333307 [details] Patch
Comment on attachment 333307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333307&action=review We need a long-term plan to move all these bits of code out of WebCore. > Source/WebCore/platform/mac/PasteboardMac.mm:661 > // This is the most innocuous event to use, per Kristen Forster. Her name is Kristin.
(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 333307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333307&action=review > > We need a long-term plan to move all these bits of code out of WebCore. > > > Source/WebCore/platform/mac/PasteboardMac.mm:661 > > // This is the most innocuous event to use, per Kristen Forster. > > Her name is Kristin. Thanks, Simon! I'll update before landing :)
Committed r228243: <https://trac.webkit.org/changeset/228243/webkit>
(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 333307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333307&action=review > > We need a long-term plan to move all these bits of code out of WebCore. > I think this can be done as part of https://bugs.webkit.org/show_bug.cgi?id=182546.
(In reply to Per Arne Vollan from comment #13) > Committed r228243: <https://trac.webkit.org/changeset/228243/webkit> This change seems to have introduced an assertion failure with API test FullscreenZoomInitialFrame.WebKit: UNEXPECTEDLY EXITED FullscreenZoomInitialFrame.WebKit ASSERTION FAILED: [NSApp isRunning] ./platform/mac/WebWindowAnimation.mm(41) : NSTimeInterval WebCore::WebWindowAnimationDurationFromDuration(NSTimeInterval) 1 0x10c52798d WTFCrash 2 0x1145c6172 WebCore::WebWindowAnimationDurationFromDuration(double) 3 0x1145c60f7 -[WebWindowScaleAnimation setDuration:] 4 0x1145c7b27 -[WebWindowScaleAnimation startAnimation] 5 0x1084b5a10 -[WebFullScreenController _startEnterFullScreenAnimationWithDuration:] 6 0x1084b3ff0 -[WebFullScreenController enterFullScreen:] 7 0x1085d407e -[WebView(WebViewInternal) _enterFullScreenForElement:] 8 0x108459531 WebChromeClient::enterFullScreenForElement(WebCore::Element&) 9 0x115960e32 WebCore::Document::requestFullScreenForElement(WebCore::Element*, WebCore::Document::FullScreenCheckType) 10 0x1159f745d WebCore::Element::webkitRequestFullscreen() 11 0x11435977a WebCore::jsElementPrototypeFunctionWebkitRequestFullScreenBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&) 12 0x11434831e long long WebCore::IDLOperation<WebCore::JSElement>::call<&(WebCore::jsElementPrototypeFunctionWebkitRequestFullScreenBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 13 0x1143480ac WebCore::jsElementPrototypeFunctionWebkitRequestFullScreen(JSC::ExecState*) 14 0x547fdf401178 15 0x10b063815 llint_entry 16 0x10b05b702 vmEntryToJavaScript 17 0x10bdd6cce JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 18 0x10bd7ca15 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 19 0x10bfe4dda JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 20 0x10bfe4eb9 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 21 0x10bfe515d JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 22 0x115468fbb WebCore::JSMainThreadExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 23 0x1154a5382 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) 24 0x115a14372 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) 25 0x115a0bd6a WebCore::EventTarget::fireEventListeners(WebCore::Event&) 26 0x115a6cbd4 WebCore::Node::handleLocalEvents(WebCore::Event&) 27 0x115a0bbdd WebCore::EventContext::handleLocalEvents(WebCore::Event&) const 28 0x115a0c094 WebCore::MouseOrFocusEventContext::handleLocalEvents(WebCore::Event&) const 29 0x115a0ca02 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) 30 0x115a0c458 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) 31 0x115a6cc2d WebCore::Node::dispatchEvent(WebCore::Event&) https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/builds/2287
Reverted r228243 for reason: Introduced an assertion failure with API test FullscreenZoomInitialFrame.WebKit Committed r228254: <https://trac.webkit.org/changeset/228254>
I believe this is a WK1 API test.
Created attachment 333387 [details] Patch
(In reply to Per Arne Vollan from comment #18) > Created attachment 333387 [details] > Patch I will work on moving these bits of code into WebKitLegacy, next.
(In reply to Per Arne Vollan from comment #18) > Created attachment 333387 [details] > Patch I am not seeing any assert failures locally with the latest patch.
Created attachment 333503 [details] Patch
(In reply to Per Arne Vollan from comment #21) > Created attachment 333503 [details] > Patch Just a rebase, since some of the code now has moved to WebKitLegacy.
Comment on attachment 333503 [details] Patch Thanks for reviewing, Simon :)
Comment on attachment 333503 [details] Patch Clearing flags on attachment: 333503 Committed r228338: <https://trac.webkit.org/changeset/228338>
All reviewed patches have been landed. Closing bug.