Bug 33828 – Crash in Page::backForwardList when using History object from a detached window

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Alexey Proskuryakov]

Created attachment 46890 [details]
test case (will crash)

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000100e43564 WebCore::Page::backForwardList() + 4
1   com.apple.WebCore             	0x0000000100a64cb8 WebCore::HistoryController::replaceState(WTF::PassRefPtr<WebCore::SerializedScriptValue>, WebCore::String const&, WebCore::String const&) + 56
2   com.apple.WebCore             	0x0000000100a64978 WebCore::History::stateObjectAdded(WTF::PassRefPtr<WebCore::SerializedScriptValue>, WebCore::String const&, WebCore::String const&, WebCore::History::StateObjectType, int&) + 440
3   com.apple.WebCore             	0x0000000100c0f32a WebCore::JSHistory::replaceState(JSC::ExecState*, JSC::ArgList const&) + 650
4   com.apple.WebCore             	0x0000000100c0e489 WebCore::jsHistoryPrototypeFunctionReplaceState(JSC::ExecState*, JSC::JSObject*, JSC::JSValue, JSC::ArgList const&) + 137

[Brady Eidson]

The real question here is... what's the expected behavior?  Should you be able to pushState() or replaceState() on a detached frame?

Clearly we could just do an early return, but what if someone does the state-API on the detached frame then reattaches it?

[Brady Eidson]

The question of "does a detached frame even have a back/forward list to manipulate?" already has an answer elsewhere in our code - calling history.length() on a detached frame will return 0 (not even 1!!)

I'm pretty sure an early return is the right way to go here.  If we later uncover an incompatibility with the spec or the real web, then we can do what's needed to fix that.

[Brady Eidson]

<rdar://problem/7556252>

[Brady Eidson]

Created attachment 46922 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 46922 [details]
Patch

"The spec really cover expected behavior"?

The test should not just have empty output - a link to the bug and a phrase like "Passed in didn't crash" would suffice.

Please add try/catch around each subtest - we want to test pushState even if replaceState raises an exception.

r=me if you fix the above nitpicks.

[Brady Eidson]

Thanks!

[Brady Eidson]

http://trac.webkit.org/changeset/53472