Bug 182553 – Assert that NSApp is not running in the WebProcess.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 333231 [details]
Patch

[Per Arne Vollan]

Created attachment 333252 [details]
Patch

[Per Arne Vollan]

Created attachment 333253 [details]
Patch

[Per Arne Vollan]

Created attachment 333290 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/37316144>

[Per Arne Vollan]

Created attachment 333295 [details]
Patch

[Simon Fraser (smfr)]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 333307 [details]
Patch

[Simon Fraser (smfr)]

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.

[Per Arne Vollan]

(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 :)

[Per Arne Vollan]

Committed r228243: <https://trac.webkit.org/changeset/228243/webkit>

[Per Arne Vollan]

(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.

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r228243 for reason:

Introduced an assertion failure with API test FullscreenZoomInitialFrame.WebKit

Committed r228254: <https://trac.webkit.org/changeset/228254>

[Per Arne Vollan]

I believe this is a WK1 API test.

[Per Arne Vollan]

Created attachment 333387 [details]
Patch

[Per Arne Vollan]

(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.

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 333503 [details]
Patch

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Comment on attachment 333503 [details]
Patch

Thanks for reviewing, Simon :)

[WebKit Commit Bot]

Comment on attachment 333503 [details]
Patch

Clearing flags on attachment: 333503

Committed r228338: <https://trac.webkit.org/changeset/228338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.