Summary: | REGRESSION: Reproducible crash going back in Back/Forward history | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
Component: | History | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson, darin, mitz | ||||||||
Priority: | P1 | Keywords: | Regression | ||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2007-01-02 21:05:53 PST
To test, try this (no guarantees): 1. Open URL: http://lists.macosforge.org/pipermail/webkit-unassigned/2007-January/date.html 2. Click message URL. 3. Hit Cmd-Right-Arrow. 4. Repeat Steps 2-3 on new messages until crash. (In reply to comment #1) > To test, try this (no guarantees): > > 1. Open URL: > http://lists.macosforge.org/pipermail/webkit-unassigned/2007-January/date.html > 2. Click message URL. > 3. Hit Cmd-LEFT-Arrow. > 4. Repeat Steps 2-3 on new messages until crash. Sorry, Step 3 should be Cmd-LEFT-Arrow to go back one page, not Cmd-Right-Arrow. Herewego, 100% repro steps 1. Open URL: http://lists.macosforge.org/pipermail/webkit-unassigned/2007-January/date.html 2. Click 1st message URL. 3. Hit Cmd-LEFT-Arrow. 4. Click 2nd message URL 5. Wait on this page - hands off your computer! - for about 5 or 6 seconds 6. Hit Cmd-LEFT-Arrow 7. Crash! A fuller backtrace at the top shows - #0 0x90a554c7 in objc_msgSend #1 0x162704c0 in ?? #2 0x004d758b in WebCore::RetainPtr<WebDataSource>::operator= at RetainPtr.h:127 #3 0x00494149 in WebDocumentLoaderMac::attachToFrame at WebDocumentLoaderMac.mm:56 #4 0x01370d83 in WebCore::DocumentLoader::setFrame at DocumentLoaderMac.mm:398 Seems to be messaging an already-dealloced objc object. bdash helped confirm this via Zombie mode, giving us - #0 0x926d89a1 in -[_NSZombie retain] #1 0x9080e1af in CFRetain #2 0x004d758b in WebCore::RetainPtr<WebDataSource>::operator= at RetainPtr.h:127 #3 0x00494149 in WebDocumentLoaderMac::attachToFrame at WebDocumentLoaderMac.mm:56 #4 0x01370d83 in WebCore::DocumentLoader::setFrame at DocumentLoaderMac.mm:398 A little more exploring tonite, then I hafta go home ;) Cmd-left-arrow is a red herring. David only noticed this and gets hit by it because using that shortcut while browsing lists of links helps this misbehavior come out. If you follow my 100% repro steps but click on the back button for the last step instead of cmd-left-arrow, the result is the same. (In reply to comment #3) > Herewego, 100% repro steps > 1. Open URL: > http://lists.macosforge.org/pipermail/webkit-unassigned/2007-January/date.html > 2. Click 1st message URL. > 3. Hit Cmd-LEFT-Arrow. > 4. Click 2nd message URL > 5. Wait on this page - hands off your computer! - for about 5 or 6 seconds > 6. Hit Cmd-LEFT-Arrow > 7. Crash! > Makes me think this has the same root cause as bug 11675. See the third paragraph in bug 11675 comment #4. Looks like something is being "reused" despite having been "finalized". Yeah, this is the same pattern as bug 11675. The "reused" object is a WebDocumentLoaderMac, whose m_detachedDataSource points to a released data source. The data source is released when the WebHTMLView is closed. setDocumentViewFromPageCache used to rely on the pageCache holding (and retaining) the data source directly. Now it is held indirectly by the pageCache via the DocumentLoader, however the latter does not retain. I'm not sure why m_detachedDataSource in WebDocumentLoaderMac is not retained (can't see how it could lead to a retain cycle), so changing this might be a possible fix. However, I still don't like the fact that HistoryItem::setHasPageCache doesn't resurrect an existing-but-scheduled-for-release pageCache, but instead creates a new one. It seems unnecessarily risky to have two pageCache objects for a single HistoryItem at a point in time (the new one and the old one scheduled for release), and I don't see any benefit in doing things this way. I think it is also the only reason that WebViews are required to support the "reopening" behavior, which is not trivial to get right. Created attachment 12210 [details]
Make PageCaches resurrect-able
As I said, I think this approach would confine the "reuse" or "resurrection" problem to HistoryItem, where it is quite manageable, and allow other objects to continue with their normal life cycle which does not necessarily support "reuse". Note that this patch does not mean "instead of reusing, throw away objects and make new ones", but rather "before making something new, see if you can find something old in the trash" :-)
Created attachment 12211 [details] Make PageCaches resurrect-able Corrected style. See comment #8. Comment on attachment 12211 [details]
Make PageCaches resurrect-able
Great patch! Mitz and I discussed some changes on IRC that he's cooking up.
Comment on attachment 12211 [details]
Make PageCaches resurrect-able
s/r-/r+/
I trust mitz to make the changes we discussed :)
Created attachment 12220 [details]
Make PageCaches resurrect-able
Did some renaming and rolled closeObjectsInPendingPageCaches() into releaseAllPendingPageCaches() (couldn't rolled the latter into releasePageCachesOrReschedule() since it's also called directly from BackForwardList). Added change log.
Comment on attachment 12220 [details]
Make PageCaches resurrect-able
Do it!
r+
Committed in r18588 |