62592 2011-06-13 13:19:40 -0700 [Chromium] WebNotification should check if ScriptExecutionContext is gone before dispatching events 2011-06-13 17:39:06 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jianli jianli dimich eric fishd levin oldest_to_newest 419802 0 jianli 2011-06-13 13:19:40 -0700 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. 419840 1 97005 jianli 2011-06-13 13:57:02 -0700 Created attachment 97005 Proposed Patch 419853 2 97005 levin 2011-06-13 14:07:04 -0700 Comment on attachment 97005 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? 419874 3 97005 eric 2011-06-13 14:32:35 -0700 Comment on attachment 97005 Proposed Patch How do we test this? 419893 4 fishd 2011-06-13 14:51:14 -0700 (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. 420070 5 jianli 2011-06-13 17:06:16 -0700 (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. 420071 6 97005 jianli 2011-06-13 17:06:43 -0700 Comment on attachment 97005 Proposed Patch Can you review again? 420101 7 jianli 2011-06-13 17:39:06 -0700 Committed as https://trac.webkit.org/changeset/88738. 97005 2011-06-13 13:57:02 -0700 2011-06-13 17:11:45 -0700 Proposed Patch 62592 text/plain 3399 jianli ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nIGIvU291cmNlL1dl YktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKaW5kZXggOWY5ZWFjNC4uNGM4Y2JhYSAxMDA3NTUKLS0t IGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9j aHJvbWl1bS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMS0wNi0xMyAgSmlhbiBMaSAg PGppYW5saUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgW0Nocm9taXVtXSBXZWJOb3RpZmljYXRpb24gc2hvdWxkIGNoZWNrIGlm IFNjcmlwdEV4ZWN1dGlvbkNvbnRleHQgaXMgZ29uZQorICAgICAgICBiZWZvcmUgZGlzcGF0Y2hp bmcgZXZlbnRzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NjI1OTIKKworICAgICAgICAqIHB1YmxpYy9XZWJOb3RpZmljYXRpb24uaDoKKyAgICAgICAg KiBzcmMvV2ViTm90aWZpY2F0aW9uLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViTm90aWZpY2F0 aW9uOjpkaXNwYXRjaERpc3BsYXlFdmVudCk6CisgICAgICAgIChXZWJLaXQ6OldlYk5vdGlmaWNh dGlvbjo6ZGlzcGF0Y2hFcnJvckV2ZW50KToKKyAgICAgICAgKFdlYktpdDo6V2ViTm90aWZpY2F0 aW9uOjpkaXNwYXRjaENsb3NlRXZlbnQpOgorICAgICAgICAoV2ViS2l0OjpXZWJOb3RpZmljYXRp b246OmRpc3BhdGNoQ2xpY2tFdmVudCk6CisgICAgICAgIChXZWJLaXQ6OldlYk5vdGlmaWNhdGlv bjo6ZGlzcGF0Y2hFdmVudCk6IEFkZGVkIGEgaGVscGVyIG1ldGhvZCB0byBjaGVjaworICAgICAg ICB0aGUgY29udGV4dCBhbmQgZGlzcGF0Y2ggYW4gZXZlbnQuCisKIDIwMTEtMDYtMTIgIEFkYW0g QmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbGV4ZXkg UHJvc2t1cnlha292LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJsaWMv V2ViTm90aWZpY2F0aW9uLmggYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJOb3Rp ZmljYXRpb24uaAppbmRleCBhODRhMDU4Li5jOTA3ODljIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L2Nocm9taXVtL3B1YmxpYy9XZWJOb3RpZmljYXRpb24uaAorKysgYi9Tb3VyY2UvV2ViS2l0 L2Nocm9taXVtL3B1YmxpYy9XZWJOb3RpZmljYXRpb24uaApAQCAtMzksNiArMzksMTAgQEAgbmFt ZXNwYWNlIFdlYkNvcmUgeyBjbGFzcyBOb3RpZmljYXRpb247IH0KIG5hbWVzcGFjZSBXVEYgeyB0 ZW1wbGF0ZSA8dHlwZW5hbWUgVD4gY2xhc3MgUGFzc1JlZlB0cjsgfQogI2VuZGlmCiAKK25hbWVz cGFjZSBXVEYgeworY2xhc3MgQXRvbWljU3RyaW5nOworfQorCiBuYW1lc3BhY2UgV2ViS2l0IHsK IAogY2xhc3MgV2ViTm90aWZpY2F0aW9uUHJpdmF0ZTsKQEAgLTEwNCw2ICsxMDgsNyBAQCBwdWJs aWM6CiAKIHByaXZhdGU6CiAgICAgdm9pZCBhc3NpZ24oV2ViTm90aWZpY2F0aW9uUHJpdmF0ZSop OworICAgIHZvaWQgZGlzcGF0Y2hFdmVudChjb25zdCBXVEY6OkF0b21pY1N0cmluZyYgdHlwZSk7 CiAgICAgV2ViTm90aWZpY2F0aW9uUHJpdmF0ZSogbV9wcml2YXRlOwogfTsKIApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViTm90aWZpY2F0aW9uLmNwcCBiL1NvdXJj ZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYk5vdGlmaWNhdGlvbi5jcHAKaW5kZXggMGMxM2NiMC4u NzhmNGU4YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViTm90aWZp Y2F0aW9uLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJOb3RpZmljYXRp b24uY3BwCkBAIC0xMTYsMjkgKzExNiwzNSBAQCB2b2lkIFdlYk5vdGlmaWNhdGlvbjo6ZGV0YWNo UHJlc2VudGVyKCkKIAogdm9pZCBXZWJOb3RpZmljYXRpb246OmRpc3BhdGNoRGlzcGxheUV2ZW50 KCkKIHsKLSAgICBSZWZQdHI8RXZlbnQ+IGV2ZW50ID0gRXZlbnQ6OmNyZWF0ZSgiZGlzcGxheSIs IGZhbHNlLCB0cnVlKTsKLSAgICBtX3ByaXZhdGUtPmRpc3BhdGNoRXZlbnQoZXZlbnQucmVsZWFz ZSgpKTsKKyAgICBkaXNwYXRjaEV2ZW50KCJkaXNwbGF5Iik7CiB9CiAKIHZvaWQgV2ViTm90aWZp Y2F0aW9uOjpkaXNwYXRjaEVycm9yRXZlbnQoY29uc3QgV2ViS2l0OjpXZWJTdHJpbmcmIC8qIGVy cm9yTWVzc2FnZSAqLykKIHsKICAgICAvLyBGSVhNRTogZXJyb3JNZXNzYWdlIG5vdCBzdXBwb3J0 ZWQgYnkgV2ViQ29yZSB5ZXQKLSAgICBSZWZQdHI8RXZlbnQ+IGV2ZW50ID0gRXZlbnQ6OmNyZWF0 ZShldmVudE5hbWVzKCkuZXJyb3JFdmVudCwgZmFsc2UsIHRydWUpOwotICAgIG1fcHJpdmF0ZS0+ ZGlzcGF0Y2hFdmVudChldmVudC5yZWxlYXNlKCkpOworICAgIGRpc3BhdGNoRXZlbnQoZXZlbnRO YW1lcygpLmVycm9yRXZlbnQpOwogfQogCiB2b2lkIFdlYk5vdGlmaWNhdGlvbjo6ZGlzcGF0Y2hD bG9zZUV2ZW50KGJvb2wgLyogYnlVc2VyICovKQogewogICAgIC8vIEZJWE1FOiBieVVzZXIgZmxh ZyBub3Qgc3VwcG9ydGVkIGJ5IFdlYkNvcmUgeWV0Ci0gICAgUmVmUHRyPEV2ZW50PiBldmVudCA9 IEV2ZW50OjpjcmVhdGUoZXZlbnROYW1lcygpLmNsb3NlRXZlbnQsIGZhbHNlLCB0cnVlKTsKLSAg ICBtX3ByaXZhdGUtPmRpc3BhdGNoRXZlbnQoZXZlbnQucmVsZWFzZSgpKTsKKyAgICBkaXNwYXRj aEV2ZW50KGV2ZW50TmFtZXMoKS5jbG9zZUV2ZW50KTsKIH0KIAogdm9pZCBXZWJOb3RpZmljYXRp b246OmRpc3BhdGNoQ2xpY2tFdmVudCgpCiB7CiAgICAgLy8gTWFrZSBzdXJlIGNsaWNrcyBvbiBu b3RpZmljYXRpb25zIGFyZSB0cmVhdGVkIGFzIHVzZXIgZ2VzdHVyZXMuCiAgICAgVXNlckdlc3R1 cmVJbmRpY2F0b3IgZ2VzdHVyZUluZGljYXRvcihEZWZpbml0ZWx5UHJvY2Vzc2luZ1VzZXJHZXN0 dXJlKTsKLSAgICBSZWZQdHI8RXZlbnQ+IGV2ZW50ID0gRXZlbnQ6OmNyZWF0ZShldmVudE5hbWVz KCkuY2xpY2tFdmVudCwgZmFsc2UsIHRydWUpOworICAgIGRpc3BhdGNoRXZlbnQoZXZlbnROYW1l cygpLmNsaWNrRXZlbnQpOworfQorCit2b2lkIFdlYk5vdGlmaWNhdGlvbjo6ZGlzcGF0Y2hFdmVu dChjb25zdCBXVEY6OkF0b21pY1N0cmluZyYgdHlwZSkKK3sKKyAgICAvLyBEbyBub3QgZGlzcGF0 Y2ggaWYgdGhlIGNvbnRleHQgaXMgZ29uZS4KKyAgICBpZiAoIW1fcHJpdmF0ZS0+c2NyaXB0RXhl Y3V0aW9uQ29udGV4dCgpKQorICAgICAgICByZXR1cm47CisKKyAgICBSZWZQdHI8RXZlbnQ+IGV2 ZW50ID0gRXZlbnQ6OmNyZWF0ZSh0eXBlLCBmYWxzZSwgdHJ1ZSk7CiAgICAgbV9wcml2YXRlLT5k aXNwYXRjaEV2ZW50KGV2ZW50LnJlbGVhc2UoKSk7CiB9CiAK