Bug 130767 – Don't handle IPC messages in event tracking run loop mode

• Component: WebKit2
• Status: REOPENED

[Alexey Proskuryakov]

In MiniBrowser, hit Command-Option-N quickly several times, and hit an assertion:

ASSERT(!_data->_keyDownEventBeingResent);

What happens here is that -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] calls _NSUnhighlightCarbonMenu(), which spins the run loop in NSEventTrackingRunLoopMode. And given that our IPC dispatches messages to main event loop in kCFRunLoopCommonModes, we get arbitrary reentrancy.

<rdar://problem/16307487>

[Alexey Proskuryakov]

Created attachment 227831 [details]
proposed patch

Changed RunLoop to only wake up itself or fire timers in default and modal run loop modes, removing event tracking one and anything else other frameworks may have registered.

- This also addresses a problem with modal alerts appearing while menus are open (setTimeout("alert(0)", 3000), and open a Safari menu).

- I didn't change other instances of kCFRunLoopCommonModes that we have (notably WebCore timers, LayerFlushScheduler, iOS WebCoreThread code, page loader scheduling, callOnMainThread()).

- Tested that drag and drop of text and images within contenteditable still works.

- Tested that showModalDialog() successfully loads a page, and timers run in this page. Tested that timers are still paused in a page that opened the dialog.

- Confusingly, I couldn't see any difference in behavior of showModalDialog() whether I allowed NSModalPanelRunLoopMode or not.

- Timers in web pages behave strangely when menus are open. They keep firing - and the page keeps repainting - when I'm slowly moving the mouse between menus or menu items, but the timers stop after I move it quickly!

The last two items concern me. Thoughts? Any other reasons why this may not be a good change?

[Darin Adler]

Comment on attachment 227831 [details]
proposed patch

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

Seems OK. Tricky to decide which run loop modes we want to omit. In the past, it was important to include NSEventTrackingRunLoopMode so that web page loading didn’t stop any time the mouse was down in another window, but it’s always been a tricky tradeoff.

> Source/WTF/wtf/RunLoop.h:149
> +    RetainPtr<CFMutableSetRef> m_additionalTimerModes;

Seems a little over-general to use a set for this given that it’s always either null or just a set with a single entry, NSModalPanelRunLoopMode. I suppose it’s nice to be general purpose, but maybe supporting just a single additional mode is OK. I can even imagine hard-coding this one mode and just using a boolean.

> Source/WebKit2/Shared/WebKit2Initialize.cpp:44
> +#if PLATFORM(MAC)
> +extern "C" CFStringRef NSModalPanelRunLoopMode;
> +#endif

I don’t think this declaration is a good idea. Giving the linker the wrong type and depending on toll-free bridging is unnecessarily ugly. We should do this in a .mm file where we can access the real constant in NSApplication.h.

Might be worth considering a helper function, maybe somewhere in WebCore/platform, that people can use to do this from cpp files.

> Source/WebKit2/Shared/WebKit2Initialize.cpp:62
>      RunLoop::initializeMainRunLoop();
> +#if PLATFORM(MAC)
> +    RunLoop::main().addModeForWakeUpAndTimers(NSModalPanelRunLoopMode);
> +#endif

I suggest exporting a function that does these two things together.

> Source/WebKit/mac/Carbon/CarbonWindowAdapter.mm:276
>      JSC::initializeThreading();
>      WTF::initializeMainThreadToProcessMainThread();
>      RunLoop::initializeMainRunLoop();
> +    RunLoop::main().addModeForWakeUpAndTimers((CFStringRef)NSModalPanelRunLoopMode);

I think this whole sequence should be a single helper function rather than putting up with having the code copied and pasted into 20 different classes.

[Darin Adler]

(In reply to comment #1)
> - Confusingly, I couldn't see any difference in behavior of showModalDialog() whether I allowed NSModalPanelRunLoopMode or not.
> 
> - Timers in web pages behave strangely when menus are open. They keep firing - and the page keeps repainting - when I'm slowly moving the mouse between menus or menu items, but the timers stop after I move it quickly!
> 
> The last two items concern me.

I think that when we have time we should investigate anomalies like these. I don’t think we’ll be able to prove this is correct just by pure logic.

[Alexey Proskuryakov]

Created attachment 227925 [details]
patch for landing

Moved initialization to WebCore/platform/MainRunLoop.h.

[WebKit Commit Bot]

Comment on attachment 227925 [details]
patch for landing

Rejecting attachment 227925 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 227925, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

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

Full output: http://webkit-queues.appspot.com/results/5203920452321280

[Alexey Proskuryakov]

Created attachment 227950 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 227950 [details]
patch for landing

Rejecting attachment 227950 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 227950, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/5109491301351424

[Alexey Proskuryakov]

Landed in <http://trac.webkit.org/r166360>.

[Alexey Proskuryakov]

Looks like this broke PLT - Safari fails to exit while spinning a nested run loop. Strange, because the run loop is spinning in NSModalPanelRunLoopMode.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 130869

[Alexey Proskuryakov]

Mostly figured out how this broke PLT, and it's quite scary. Here is what happens:

1. When PLT is done, it closes its windows with (essentially) -[NSWindow performClose]. This results in sending an IPC message to WebProcess, but it also spins a nested run loop *in event tracking mode* to highlight the window close button.
2. WebProcess is very quick to close the page, so a response arrives while spinning the run loop.
3. The response results in calling -[NSWindow close].
4. Then PLT tries to terminate Safari, and calls -[NSApplication terminate].
5. -[NSApplication terminate] tries to close all documents (not all windows), and while waiting for that, it spins run loop *in modal dialog mode*.
6. While in this mode, an NSEvent internal to AppKit is delivered, which confirms that all documents agreed to termination, and termination continues.

Now, if we don't deliver IPC responses in event tracking mode - or if closing the page takes longer than the close button highlight is visible - then the response is delivered later while in modal dialog mode, and calling -close doesn't convince NSApplication that the document got closed. So, NSApplication keeps waiting.

While I'm not sure how to fix this, It seems very likely that this cannot be fixed in WebKit alone.

[Csaba Osztrogonác]

Comment on attachment 227831 [details]
proposed patch

Cleared Darin Adler's review+ from obsolete attachment 227831 [details] so that this bug does not appear in http://webkit.org/pending-commit.