146093 2015-06-17 19:00:09 -0700 Crash under WebCore::DOMWindow::dispatchMessageEventWithOriginCheck attempting to log console message 2015-06-18 11:17:44 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck ap cdumez commit-queue joepeck timothy oldest_to_newest 1102752 0 joepeck 2015-06-17 19:00:09 -0700 * SUMMARY Crash under WebCore::DOMWindow::dispatchMessageEventWithOriginCheck attempting to log console message. * CRASH SNIPPET Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000008 Triggered by Thread: 0 Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed: 0 WebCore::PageConsoleClient::addMessage(JSC::MessageSource, JSC::MessageLevel, WTF::String const&, WTF::String const&, unsigned int, unsigned int, WTF::RefPtr<Inspector::ScriptCallStack>&&, JSC::ExecState*, unsigned long) + 288 (PageConsoleClient.cpp:138) 1 WebCore::PageConsoleClient::addMessage(JSC::MessageSource, JSC::MessageLevel, WTF::String const&, WTF::String const&, unsigned int, unsigned int, WTF::RefPtr<Inspector::ScriptCallStack>&&, JSC::ExecState*, unsigned long) + 244 (StdLibExtras.h:337) 2 WebCore::PageConsoleClient::addMessage(JSC::MessageSource, JSC::MessageLevel, WTF::String const&, WTF::RefPtr<Inspector::ScriptCallStack>&&) + 40 (PageConsoleClient.cpp:119) 3 WebCore::DOMWindow::dispatchMessageEventWithOriginCheck(WebCore::SecurityOrigin*, WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<Inspector::ScriptCallStack>) + 928 (DOMWindow.cpp:941) 4 WebCore::DOMWindow::postMessageTimerFired(WebCore::PostMessageTimer&) + 148 (DOMWindow.cpp:931) 5 WebCore::PostMessageTimer::fired() + 24 (DOMWindow.cpp:173) 6 WebCore::ThreadTimers::sharedTimerFiredInternal() + 144 (ThreadTimers.cpp:132) 7 WebCore::timerFired(__CFRunLoopTimer*, void*) + 32 (SharedTimerCF.cpp:82) 1102753 1 joepeck 2015-06-17 19:00:29 -0700 <rdar://problem/21380687> 1102756 2 joepeck 2015-06-17 19:03:47 -0700 I was able to reproduce this once, in the debugger, and verify that a caller of DOMWindow::console() was not handling the possible nullptr result. PageConsoleClient* DOMWindow::console() const { if (!isCurrentlyDisplayedInFrame()) return nullptr; return m_frame->page() ? &m_frame->page()->console() : nullptr; } I was unable to reproduce this reliably though. The test where I was able to get it was something like this, in the LayoutTests/http/tests/security/postMessage directory. <script> setTimeout(function() { var frame = document.getElementById("iframe-localhost"); var frameWindow = frame.contentWindow; var postMessageFunction = frameWindow.postMessage; console.log("here0", frameWindow, postMessage) postMessageFunction.call(frameWindow, "message", "http://www.example.com"); setTimeout(function() { console.log("here1", frameWindow, postMessage) postMessageFunction.call(frameWindow, "message", "http://www.example.com"); gc(); }, 1000); setTimeout(function() { frame.parentElement.removeChild(frame); console.log("here2", frameWindow, postMessage) postMessageFunction.call(frameWindow, "message", "http://www.example.com"); }, 2000); setTimeout(function() { console.log("here2", frameWindow, postMessage) postMessageFunction.call(frameWindow, "message", "http://www.example.com"); }, 3000); }, 0); </script> <iframe src="http://localhost:8000/security/postMessage/resources/post-message-listener.html" id="iframe-localhost" width="800" height="300" style="border: 1px solid black;"></iframe> Being unable to reliably reproduce this, and since it is a very straight forward null check, I'm going to go ahead with a patch. If someone wants me to try more for a test, let me know and I'll spend more time looking into it. 1102757 3 255061 joepeck 2015-06-17 19:07:24 -0700 Created attachment 255061 [PATCH] Proposed Fix 1102781 4 ap 2015-06-17 19:59:49 -0700 This feels like it should be testable. Perhaps Chris has an idea how? I would try going a garbage collection after the frame is removed, not before. Also, postMessage is likely red herring, I'd be trying a synchronous test along the lines of what Tim sent you in an e-mail. 1102786 5 joepeck 2015-06-17 20:20:32 -0700 (In reply to comment #4) > This feels like it should be testable. Perhaps Chris has an idea how? > > I would try going a garbage collection after the frame is removed, not > before. Also, postMessage is likely red herring, I'd be trying a synchronous > test along the lines of what Tim sent you in an e-mail. Yep, this is almost identical to the test Tim sent me. postMessage is what gets us down the DOMWindow::dispatchMessageEventWithOriginCheck path, so I think it is required here? Though I had reproduced this once, in trying to re-reproduce it I threw in a number of gc() calls using <script src="/js-test-resources/js-test-pre.js"></script> and had no success =(. 1102787 6 joepeck 2015-06-17 20:22:02 -0700 Oh, the fact that there was a gc() in what I had above was an accident. I had tested a gc after the frame was removed. 1102977 7 255061 commit-queue 2015-06-18 11:17:38 -0700 Comment on attachment 255061 [PATCH] Proposed Fix Clearing flags on attachment: 255061 Committed r185712: <http://trac.webkit.org/changeset/185712> 1102978 8 commit-queue 2015-06-18 11:17:44 -0700 All reviewed patches have been landed. Closing bug. 255061 2015-06-17 19:07:24 -0700 2015-06-18 11:17:38 -0700 [PATCH] Proposed Fix crash-fix.patch text/plain 1976 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBlYzVkOWMxLi40MDY5M2RiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTUtMDYtMTcgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwor ICAgICAgICBDcmFzaCB1bmRlciBXZWJDb3JlOjpET01XaW5kb3c6OmRpc3BhdGNoTWVzc2FnZUV2 ZW50V2l0aE9yaWdpbkNoZWNrIGF0dGVtcHRpbmcgdG8gbG9nIGNvbnNvbGUgbWVzc2FnZQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2MDkzCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwYWdlL0RPTVdp bmRvdy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpET01XaW5kb3c6OmRpc3BhdGNoTWVzc2FnZUV2 ZW50V2l0aE9yaWdpbkNoZWNrKToKKyAgICAgICAgVGhlIGNvbnNvbGUgY291bGQgYmUgbnVsbCBz byBudWxsIGNoZWNrIGl0cyB1c2UuCisKIDIwMTUtMDYtMTYgIEFsZXggQ2hyaXN0ZW5zZW4gIDxh Y2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4KIAogICAgICAgICBbQ29udGVudCBFeHRlbnNpb25zXSBJ bXBsZW1lbnQgYnJhbmNoIGNvbXBhY3Rpb24gZm9yIERGQSBieXRlY29kZS4KZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL3BhZ2UvRE9NV2luZG93LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BhZ2Uv RE9NV2luZG93LmNwcAppbmRleCA5ODA1OTQ2Li5jZWE0MGE4IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wYWdlL0RPTVdpbmRvdy5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9ET01X aW5kb3cuY3BwCkBAIC05MzQsOSArOTM0LDEwIEBAIHZvaWQgRE9NV2luZG93OjpkaXNwYXRjaE1l c3NhZ2VFdmVudFdpdGhPcmlnaW5DaGVjayhTZWN1cml0eU9yaWdpbiogaW50ZW5kZWRUYXJnCiAg ICAgaWYgKGludGVuZGVkVGFyZ2V0T3JpZ2luKSB7CiAgICAgICAgIC8vIENoZWNrIHRhcmdldCBv cmlnaW4gbm93IHNpbmNlIHRoZSB0YXJnZXQgZG9jdW1lbnQgbWF5IGhhdmUgY2hhbmdlZCBzaW5j ZSB0aGUgdGltZXIgd2FzIHNjaGVkdWxlZC4KICAgICAgICAgaWYgKCFpbnRlbmRlZFRhcmdldE9y aWdpbi0+aXNTYW1lU2NoZW1lSG9zdFBvcnQoZG9jdW1lbnQoKS0+c2VjdXJpdHlPcmlnaW4oKSkp IHsKLSAgICAgICAgICAgIFN0cmluZyBtZXNzYWdlID0gIlVuYWJsZSB0byBwb3N0IG1lc3NhZ2Ug dG8gIiArIGludGVuZGVkVGFyZ2V0T3JpZ2luLT50b1N0cmluZygpICsKLSAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIi4gUmVjaXBpZW50IGhhcyBvcmlnaW4gIiArIGRvY3VtZW50KCktPnNl Y3VyaXR5T3JpZ2luKCktPnRvU3RyaW5nKCkgKyAiLlxuIjsKLSAgICAgICAgICAgIGNvbnNvbGUo KS0+YWRkTWVzc2FnZShNZXNzYWdlU291cmNlOjpTZWN1cml0eSwgTWVzc2FnZUxldmVsOjpFcnJv ciwgbWVzc2FnZSwgc3RhY2tUcmFjZSk7CisgICAgICAgICAgICBpZiAoUGFnZUNvbnNvbGVDbGll bnQqIHBhZ2VDb25zb2xlID0gY29uc29sZSgpKSB7CisgICAgICAgICAgICAgICAgU3RyaW5nIG1l c3NhZ2UgPSBtYWtlU3RyaW5nKCJVbmFibGUgdG8gcG9zdCBtZXNzYWdlIHRvICIsIGludGVuZGVk VGFyZ2V0T3JpZ2luLT50b1N0cmluZygpLCAiLiBSZWNpcGllbnQgaGFzIG9yaWdpbiAiLCBkb2N1 bWVudCgpLT5zZWN1cml0eU9yaWdpbigpLT50b1N0cmluZygpLCAiLlxuIik7CisgICAgICAgICAg ICAgICAgcGFnZUNvbnNvbGUtPmFkZE1lc3NhZ2UoTWVzc2FnZVNvdXJjZTo6U2VjdXJpdHksIE1l c3NhZ2VMZXZlbDo6RXJyb3IsIG1lc3NhZ2UsIHN0YWNrVHJhY2UpOworICAgICAgICAgICAgfQog ICAgICAgICAgICAgcmV0dXJuOwogICAgICAgICB9CiAgICAgfQo=