Bug 146093 – Crash under WebCore::DOMWindow::dispatchMessageEventWithOriginCheck attempting to log console message

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

* 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)

[Joseph Pecoraro]

<rdar://problem/21380687>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 255061 [details]
[PATCH] Proposed Fix

[Alexey Proskuryakov]

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.

[Joseph Pecoraro]

(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 =(.

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 255061 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 255061

Committed r185712: <http://trac.webkit.org/changeset/185712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.