Summary: | REGRESSION: Crash closing page with frames after selection | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Lilek <dev+webkit> | ||||||||
Component: | New Bugs | Assignee: | 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
Matt Lilek
2006-11-30 16:41:59 PST
Created attachment 11699 [details]
testcase
Created attachment 11700 [details]
crashlog
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. I'll have a look. 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. 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. 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. *** Bug 11863 has been marked as a duplicate of this bug. *** This bug affects GMail as well. See Bug 11863. (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). 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 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.
Committed revision 18460. |