[Chromium] WebNotification should check if ScriptExecutionContext is gone before dispatching events

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

WebNotification should check if ScriptExecutionContext is gone before dispatching events. Otherwise, we will see crashes like the following:

0x0241d382	 [chrome.dll	 - v8abstracteventlistener.cpp:76]	WebCore::V8AbstractEventListener::handleEvent(WebCore::ScriptExecutionContext *,WebCore::Event *)
0x0232cc87	 [chrome.dll	 - eventtarget.cpp:389]	WebCore::EventTarget::fireEventListeners(WebCore::Event *,WebCore::EventTargetData *,WTF::Vector<WebCore::RegisteredEventListener,1> &)
0x0232cbbe	 [chrome.dll	 - eventtarget.cpp:358]	WebCore::EventTarget::fireEventListeners(WebCore::Event *)
0x0232cb5a	 [chrome.dll	 - eventtarget.cpp:340]	WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
0x025e21cb	 [chrome.dll	 - webnotification.cpp:134]	WebKit::WebNotification::dispatchCloseEvent(bool)
0x01cb16c9	 [chrome.dll	 - notification_provider.cc:154]	NotificationProvider::OnClose(int,bool)
0x01cb1103	 [chrome.dll	 - notification_provider.cc:88]	NotificationProvider::OnMessageReceived(IPC::Message const &)
0x01ca1cda	 [chrome.dll	 - render_view.cc:592]	RenderView::OnMessageReceived(IPC::Message const &)
0x01c8a77f	 [chrome.dll	 - message_router.cc:46]	MessageRouter::RouteMessage(IPC::Message const &)
0x01c8a759	 [chrome.dll	 - message_router.cc:38]	MessageRouter::OnMessageReceived(IPC::Message const &)
0x01c8455e	 [chrome.dll	 - child_thread.cc:175]	ChildThread::OnMessageReceived(IPC::Message const &)
0x01edd3f2	 [chrome.dll	 - task.h:338]	RunnableMethod<ProfileWriter,void ( ProfileWriter::*)(GURL const &),Tuple1<GURL> >::Run()
0x01c451a4	 [chrome.dll	 - message_loop.cc:102]	`anonymous namespace'::TaskClosureAdapter::Run()
0x01c45c98	 [chrome.dll	 - message_loop.cc:482]	MessageLoop::RunTask(MessageLoop::PendingTask const &)
0x01c45d1b	 [chrome.dll	 - message_loop.cc:500]	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x01c46096	 [chrome.dll	 - message_loop.cc:691]	MessageLoop::DoWork()
0x01c5094f	 [chrome.dll	 - message_pump_default.cc:50]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x01c45bb1	 [chrome.dll	 - message_loop.cc:449]	MessageLoop::RunInternal()
0x01c45b36	 [chrome.dll	 - message_loop.cc:422]	MessageLoop::RunHandler()
0x01c45a2a	 [chrome.dll	 - message_loop.cc:346]	MessageLoop::Run()
0x01c9e3fd	 [chrome.dll	 - renderer_main.cc:235]	RendererMain(MainFunctionParams const &)
0x01c348ef	 [chrome.dll	 - chrome_main.cc:815]	ChromeMain
0x004022b8	 [chrome.exe	 - client_util.cc:254]	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x0040595a	 [chrome.exe	 - chrome_exe_main_win.cc:46]	wWinMain
0x00454f9f	 [chrome.exe	 - crt0.c:263]	__tmainCRTStartup
0x7c817076	 [kernel32.dll	 + 0x00017076]	BaseProcessStart

This is a regression from http://trac.webkit.org/changeset/83900. It helps to expose the underlying problem in WebNotification code.

[Jian Li]

Created attachment 97005 [details]
Proposed Patch

[David Levin]

Comment on attachment 97005 [details]
Proposed Patch

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

> Source/WebKit/chromium/src/WebNotification.cpp:144
> +    if (!m_private->scriptExecutionContext())

1. How does this get cleared?
2. How did http://trac.webkit.org/changeset/83900 expose this issue?

[Eric Seidel (no email)]

Comment on attachment 97005 [details]
Proposed Patch

How do we test this?

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> (From update of attachment 97005 [details])
> How do we test this?

This can be tested via webkit_unit_tests.  Just add a gtest in chromium/tests/.  You probably need something like WebFrameTest from WebFrameTests.cpp so that you can setup a real document.

[Jian Li]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 97005 [details] [details])
> > How do we test this?
> 
> This can be tested via webkit_unit_tests.  Just add a gtest in chromium/tests/.  You probably need something like WebFrameTest from WebFrameTests.cpp so that you can setup a real document.

The crash seems to be caused by the timing between closing main page and closing notification balloon. If it happens that the code is executed in the following order:
  NotificationCenter::disconnectFrame
  Notification::contextDestroyed
  NotificationProvider::OnClose
  NotificationProvider::~NotificationProvider
we will hit the crash. NotificationCenter::disconnectFrame calls m_notificationPresenter->cancelRequestsForPermission (just dummy stub) and sets m_notificationPresenter to 0. Then when Notification::contextDestroyed is called, we cannot delegate to presenter()->notificationObjectDestroyed since presenter() is NULL. After that, when NotificationProvider::OnClose is called for the balloon close, we still have valid WebNotification in the list and will call from WebNotification into WebCore::Notification to dispatch the event, but Notification::m_scriptExecutionContext is gone.

I think the good fix is to implement cancelRequestsForPermission and delegate into NotificationProvider to clear the list. However, this change involves 2-side change and will take some time to develop and checkin. So probably we should land this quick fix so that it can be merged into release branches.

I am not able to repro the crash since it only happens with certain timing. Sorry for not able to provide a consistent test for it.

[Jian Li]

Comment on attachment 97005 [details]
Proposed Patch

Can you review again?

[Jian Li]

Committed as https://trac.webkit.org/changeset/88738.