Bug 62592

Summary: [Chromium] WebNotification should check if ScriptExecutionContext is gone before dispatching events
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore Misc.Assignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, eric, fishd, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch levin: review+, jianli: commit-queue-

Jian Li
Reported 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.
Attachments
Proposed Patch (3.32 KB, patch)
2011-06-13 13:57 PDT, Jian Li 		levin: review+
jianli: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jian Li
Comment 1 2011-06-13 13:57:02 PDT
Created attachment 97005 [details] Proposed Patch
David Levin
Comment 2 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?
Eric Seidel (no email)
Comment 3 2011-06-13 14:32:35 PDT
Comment on attachment 97005 [details] Proposed Patch How do we test this?
Darin Fisher (:fishd, Google)
Comment 4 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.
Jian Li
Comment 5 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.
Jian Li
Comment 6 2011-06-13 17:06:43 PDT
Comment on attachment 97005 [details] Proposed Patch Can you review again?
Jian Li
Comment 7 2011-06-13 17:39:06 PDT
Committed as https://trac.webkit.org/changeset/88738.
Note You need to log in before you can comment on or make changes to this bug.