Bug 11729

Summary: REGRESSION: Crash closing page with frames after selection
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ddkilzer, ggaren, miles_b, mitz
Priority: P1 Keywords: GoogleBug, HasReduction, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
testcase
none
crashlog
none
Minimal fix darin: review+

Description Matt Lilek 2006-11-30 16:41:59 PST
This is very similar to bug 11710.

Open the testcase, select something in one of the frames, close the tab/window and WebKit will crash.
Comment 1 Matt Lilek 2006-11-30 16:43:43 PST
Created attachment 11699 [details]
testcase
Comment 2 Matt Lilek 2006-11-30 16:45:17 PST
Created attachment 11700 [details]
crashlog
Comment 3 mitz 2006-12-01 00:47:46 PST
The crash happens because the subframe points to a Page that's already been destroyed. The Page destructor traverses the frame tree and calls pageDestroyed() on every subframe, but that's too late since -[_webView close] tears down the tree before deleting the Page, so the Page destructor actually doesn't reach any subframes.

It seems to me that Frame::pageDestroyed() could be Frame::detachedFromTree() and be called when the tree is torn down.
Comment 4 Geoffrey Garen 2006-12-03 17:29:07 PST
I'll have a look.
Comment 5 Geoffrey Garen 2006-12-03 18:10:17 PST
Hmmm... I can't reproduce this with r17994.
Comment 6 Matt Lilek 2006-12-03 22:41:08 PST
I can still get this to happen in the r17993 nightly.  The most reliable way I've found is to launch the nightly, open a new tab, go directly to the attachment on this bug, select the text and close the tab which will cause a beachball and crash.
Comment 7 mitz 2006-12-03 22:48:06 PST
With a debug build you must have another window open in order to reproduce, because of this:

    if (allPages->isEmpty()) {
        Frame::endAllLifeSupport();
#ifndef NDEBUG
        // Force garbage collection, to release all Nodes before exiting.
        m_mainFrame = 0;
#endif
    }

Clearing m_mainFrame causes it (and the subframe it's holding onto via EventHandler) to be deleted while the Page is still valid.
Comment 8 Miles Bainbridge 2006-12-16 22:11:50 PST
I'm getting this exact crash in build 18260.  I'm experiencing this in an Apple internal implementation of PeopleSoft 8.  It seems to be 100% reproducible in this situation, though so far I've only tested it on a PPC machine.  Due to the nature of the crash, it makes this build of WebKit unusable for PeopleSoft 8-based databases.
Comment 9 David Kilzer (:ddkilzer) 2006-12-17 18:04:48 PST
*** Bug 11863 has been marked as a duplicate of this bug. ***
Comment 10 David Kilzer (:ddkilzer) 2006-12-17 18:07:39 PST
This bug affects GMail as well.  See Bug 11863.

Comment 11 David Kilzer (:ddkilzer) 2006-12-20 20:07:52 PST
(In reply to comment #9)
> *** Bug 11863 has been marked as a duplicate of this bug. ***

(In reply to comment #10)
> This bug affects GMail as well.  See Bug 11863.

See Bug 11859 Comment #6 for a different stack trace when closing a GMail compose window using a locally-built debug build of WebKit r18370 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).

Comment 12 mitz 2006-12-27 01:22:19 PST
Created attachment 12055 [details]
Minimal fix

I realize that you are going to address this bug as part of some bigger change, but please consider this fix for now. I don't know how to make an automated test.
Comment 13 Darin Adler 2006-12-27 22:17:09 PST
Comment on attachment 12055 [details]
Minimal fix

The function name pageDestroyed is no longer a good one if it's used in these cases where the page is not being destroyed.

And if it's an error to call setView(0) without also calling pageDestroyed(), perhaps a little refactoring of that is in order to make that mistake impossible.

Since FrameLoader::detachFromParent does some of the same kind of work that removeChild does, I'd like to see some refactoring so that code does not exist in two different place.

It's a bad thing that the page's frame count is now managed both by Frame and FrameTree -- that should not be a shared responsibility.

Still, r=me, because we want this fixed in the tree; lets see if we can clean things up a bit after the fact.

If maybe Geoff is doing some bigger fix maybe that will do the trick.
Comment 14 David Kilzer (:ddkilzer) 2006-12-28 15:48:39 PST
Committed revision 18460.