CRASH doing open() on destroyed window

• Component: Page Loading
• Status: RESOLVED FIXED

[Eric Roman]

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

[Eric Roman]

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

[Eric Roman]

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

[Mark Rowe (bdash)]

<rdar://problem/6012706>

[Sam Weinig]

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]

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

[Alexey Proskuryakov]

See also: bug 15707.

[Eric Seidel (no email)]

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]

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 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]

Updated with Eric's comment and landed in r35321.