Bug 19588

Summary:

CRASH doing open() on destroyed window

Product:

WebKit

Reporter:

Eric Roman <eroman>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

jchaffraix

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Reproducible test case for crash -- click the button	none
Crash dump from running the above test on nightly webkit r34581	none
Proposed fix: add null check	eric: review-
Updated version: use top() and cover the other crash pointed by Eric	eric: review+

Eric Roman

Reported 2008-06-16 10:18:59 PDT

Remove iframe from parent, and then do a open() on its contentWindow -- reproducible crash on Safari 3.1 and nightly.

Attachments
Reproducible test case for crash -- click the button (461 bytes, text/html) 2008-06-16 10:20 PDT, Eric Roman	no flags	Details
Crash dump from running the above test on nightly webkit r34581 (27.77 KB, text/plain) 2008-06-16 10:23 PDT, Eric Roman	no flags	Details
Proposed fix: add null check (3.31 KB, patch) 2008-07-21 16:57 PDT, Julien Chaffraix	eric: review-	Details Formatted Diff Diff
Updated version: use top() and cover the other crash pointed by Eric (6.75 KB, patch) 2008-07-22 14:21 PDT, Julien Chaffraix	eric: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Eric Roman

Comment 1 2008-06-16 10:20:26 PDT

Created attachment 21730 [details] Reproducible test case for crash -- click the button

Eric Roman

Comment 2 2008-06-16 10:23:01 PDT

Created attachment 21731 [details] Crash dump from running the above test on nightly webkit r34581

Mark Rowe (bdash)

Comment 3 2008-06-16 13:46:02 PDT

<rdar://problem/6012706>

Sam Weinig

Comment 4 2008-07-16 15:19:29 PDT

This is not a new crash. Using bisect-builds, I was able to reproduce this all the way back to r29753, the beginning of builds available to the tool.

Julien Chaffraix

Comment 5 2008-07-21 16:57:30 PDT

Created attachment 22415 [details] Proposed fix: add null check

Alexey Proskuryakov

Comment 6 2008-07-21 22:46:02 PDT

See also: bug 15707.

Eric Seidel (no email)

Comment 7 2008-07-22 08:32:59 PDT

Comment on attachment 22415 [details] Proposed fix: add null check Why shouldn't you just use "top()" here? It seems "top()" wouldn't crash. Seems like the end of the function doesn't check against null pages either... so that will need to be fixed and a test case added too.

Julien Chaffraix

Comment 8 2008-07-22 14:21:05 PDT

Created attachment 22432 [details] Updated version: use top() and cover the other crash pointed by Eric > Why shouldn't you just use "top()" here? It seems "top()" wouldn't crash. top() is indeed fine. > Seems like the end of the function doesn't check against null pages either... > so that will need to be fixed and a test case added too. True, I have fixed that too including a layout test.

Eric Seidel (no email)

Comment 9 2008-07-23 13:17:34 PDT

Comment on attachment 22432 [details] Updated version: use top() and cover the other crash pointed by Eric Looks fine, but: if (page) { 184 for (Frame* frame = page should change to be an early return: if (!page) return 0;

Julien Chaffraix

Comment 10 2008-07-24 00:47:06 PDT

Updated with Eric's comment and landed in r35321.

Note You need to log in before you can comment on or make changes to this bug.