Bug 62592 - [Chromium] WebNotification should check if ScriptExecutionContext is gone before dispatching events
Summary: [Chromium] WebNotification should check if ScriptExecutionContext is gone bef...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-13 13:19 PDT by Jian Li
Modified: 2011-06-13 17:39 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Patch (3.32 KB, patch)
2011-06-13 13:57 PDT, Jian Li
levin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2011-06-13 13:19:40 PDT
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.
Comment 1 Jian Li 2011-06-13 13:57:02 PDT
Created attachment 97005 [details]
Proposed Patch
Comment 2 David Levin 2011-06-13 14:07:04 PDT
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?
Comment 3 Eric Seidel (no email) 2011-06-13 14:32:35 PDT
Comment on attachment 97005 [details]
Proposed Patch

How do we test this?
Comment 4 Darin Fisher (:fishd, Google) 2011-06-13 14:51:14 PDT
(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.
Comment 5 Jian Li 2011-06-13 17:06:16 PDT
(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.
Comment 6 Jian Li 2011-06-13 17:06:43 PDT
Comment on attachment 97005 [details]
Proposed Patch

Can you review again?
Comment 7 Jian Li 2011-06-13 17:39:06 PDT
Committed as https://trac.webkit.org/changeset/88738.