Bug 33828 - Crash in Page::backForwardList when using History object from a detached window
Summary: Crash in Page::backForwardList when using History object from a detached window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-01-18 23:29 PST by Alexey Proskuryakov
Modified: 2010-01-19 14:14 PST (History)
2 users (show)

See Also:


Attachments
test case (will crash) (245 bytes, text/html)
2010-01-18 23:29 PST, Alexey Proskuryakov
no flags Details
Patch (3.29 KB, patch)
2010-01-19 10:06 PST, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-18 23:29:03 PST
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
Comment 1 Brady Eidson 2010-01-19 09:44:39 PST
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?
Comment 2 Brady Eidson 2010-01-19 09:48:51 PST
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.
Comment 3 Brady Eidson 2010-01-19 09:49:25 PST
<rdar://problem/7556252>
Comment 4 Brady Eidson 2010-01-19 10:06:00 PST
Created attachment 46922 [details]
Patch
Comment 5 Alexey Proskuryakov 2010-01-19 10:10:12 PST
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.
Comment 6 Brady Eidson 2010-01-19 10:14:49 PST
Thanks!
Comment 7 Brady Eidson 2010-01-19 10:19:14 PST
http://trac.webkit.org/changeset/53472