Bug 12087 – REGRESSION: Reproducible crash going back in Back/Forward history

• Component: History
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

I'm seeing intermittent (so far, two) crashes when going back in history using Cmd-Right-Arrow.  The first crash happened after only a few (less than 10) pages were loaded after a restart, while the second crash happened after dozens of page loads.  In both cases, I was reading webkit-unassigned archives, had clicked on a specific message, then hit Cmd-Right-Arrow to go back to the Jan 2007 by-date page.

Here's the first stack trace:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xe5895590

Thread 0 Crashed:
0   libobjc.A.dylib             0x90a564c7 objc_msgSend + 23
1   com.apple.WebKit            0x003d562f WebCore::RetainPtr<WebDataSource>::operator=(WebDataSource*) + 23 (RetainPtr.
h:128)
2   com.apple.WebKit            0x0039323f WebDocumentLoaderMac::attachToFrame() + 145 (WebDocumentLoaderMac.mm:57)
3   com.apple.WebCore           0x0136d33b WebCore::DocumentLoader::setFrame(WebCore::Frame*) + 119 (DocumentLoaderMac.m
m:399)
4   com.apple.WebCore           0x01392b67 WebCore::FrameLoader::setPolicyDocumentLoader(WebCore::DocumentLoader*) + 129
 (FrameLoader.cpp:1852)
5   com.apple.WebCore           0x01372d4c WebCore::FrameLoader::load(WebCore::DocumentLoader*, WebCore::FrameLoadType, 
WTF::PassRefPtr<WebCore::FormState>) + 312 (FrameLoaderMac.mm:272)
6   com.apple.WebCore           0x01397e83 WebCore::FrameLoader::loadItem(WebCore::HistoryItem*, WebCore::FrameLoadType)
 + 879 (FrameLoader.cpp:2796)
7   com.apple.WebCore           0x013986b9 WebCore::FrameLoader::recursiveGoToItem(WebCore::HistoryItem*, WebCore::Histo
ryItem*, WebCore::FrameLoadType) + 1031 (FrameLoader.cpp:2952)
8   com.apple.WebCore           0x013987a7 WebCore::FrameLoader::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType)
 + 205 (FrameLoader.cpp:2901)
9   com.apple.WebCore           0x0119e12c WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 308 
(Page.cpp:133)
10  com.apple.WebCore           0x0119e1c7 WebCore::Page::goBack() + 63 (Page.cpp:109)
11  com.apple.WebKit            0x00365af1 -[WebView goBack] + 23 (WebView.mm:2158)
12  com.apple.WebKit            0x0035c27d -[WebFrameView _goBack] + 57 (WebFrameView.mm:556)
13  com.apple.WebKit            0x0035d4bb -[WebFrameView keyDown:] + 2085 (WebFrameView.mm:809)
14  com.apple.AppKit            0x932f2b59 forwardMethod + 82
15  com.apple.AppKit            0x932f2b59 forwardMethod + 82
16  com.apple.AppKit            0x932f2b59 forwardMethod + 82
17  com.apple.AppKit            0x934483a1 -[NSControl keyDown:] + 122
18  com.apple.WebKit            0x003403a1 -[WebHTMLView keyDown:] + 553 (WebHTMLView.m:3410)
19  com.apple.AppKit            0x9335cbe1 -[NSWindow sendEvent:] + 7377
20  com.apple.Safari            0x000230c6 0x1000 + 139462
21  com.apple.AppKit            0x9334e350 -[NSApplication sendEvent:] + 5023
22  com.apple.Safari            0x00022c56 0x1000 + 138326
23  com.apple.AppKit            0x93278dfe -[NSApplication run] + 547
24  com.apple.AppKit            0x9326cd2f NSApplicationMain + 573
25  com.apple.Safari            0x0005f54a 0x1000 + 386378
26  com.apple.Safari            0x0005f471 0x1000 + 386161


Here's the second stack trace:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000000

Thread 0 Crashed:
0   libobjc.A.dylib                	0x90a564c7 objc_msgSend + 23
1   com.apple.WebKit               	0x003d562f WebCore::RetainPtr<WebDataSource>::operator=(WebDataSource*) + 23 (RetainPtr.h:128)
2   com.apple.WebKit               	0x0039323f WebDocumentLoaderMac::attachToFrame() + 145 (WebDocumentLoaderMac.mm:57)
3   com.apple.WebCore              	0x0136d33b WebCore::DocumentLoader::setFrame(WebCore::Frame*) + 119 (DocumentLoaderMac.mm:399)
4   com.apple.WebCore              	0x01392b67 WebCore::FrameLoader::setPolicyDocumentLoader(WebCore::DocumentLoader*) + 129 (FrameLoader.cpp:1852)
5   com.apple.WebCore              	0x01372d4c WebCore::FrameLoader::load(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>) + 312 (FrameLoaderMac.mm:272)
6   com.apple.WebCore              	0x01397e83 WebCore::FrameLoader::loadItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 879 (FrameLoader.cpp:2796)
7   com.apple.WebCore              	0x013986b9 WebCore::FrameLoader::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType) + 1031 (FrameLoader.cpp:2952)
8   com.apple.WebCore              	0x013987a7 WebCore::FrameLoader::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 205 (FrameLoader.cpp:2901)
9   com.apple.WebCore              	0x0119e12c WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 308 (Page.cpp:133)
10  com.apple.WebCore              	0x0119e1c7 WebCore::Page::goBack() + 63 (Page.cpp:109)
11  com.apple.WebKit               	0x00365af1 -[WebView goBack] + 23 (WebView.mm:2158)
12  com.apple.WebKit               	0x0035c27d -[WebFrameView _goBack] + 57 (WebFrameView.mm:556)
13  com.apple.WebKit               	0x0035d4bb -[WebFrameView keyDown:] + 2085 (WebFrameView.mm:809)
14  com.apple.AppKit               	0x932f2b59 forwardMethod + 82
15  com.apple.AppKit               	0x932f2b59 forwardMethod + 82
16  com.apple.AppKit               	0x932f2b59 forwardMethod + 82
17  com.apple.AppKit               	0x934483a1 -[NSControl keyDown:] + 122
18  com.apple.WebKit               	0x003403a1 -[WebHTMLView keyDown:] + 553 (WebHTMLView.m:3410)
19  com.apple.AppKit               	0x9335cbe1 -[NSWindow sendEvent:] + 7377
20  com.apple.Safari               	0x000230c6 0x1000 + 139462
21  com.apple.AppKit               	0x9334e350 -[NSApplication sendEvent:] + 5023
22  com.apple.Safari               	0x00022c56 0x1000 + 138326
23  com.apple.AppKit               	0x93278dfe -[NSApplication run] + 547
24  com.apple.AppKit               	0x9326cd2f NSApplicationMain + 573
25  com.apple.Safari               	0x0005f54a 0x1000 + 386378
26  com.apple.Safari               	0x0005f471 0x1000 + 386161

Both crashes occurred on a locally-built debug build of WebKit r18541 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037).

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[Brady Eidson]

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!

[Brady Eidson]

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  ;)

[Brady Eidson]

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.

[Brady Eidson]

Comment on attachment 12211 [details]
Make PageCaches resurrect-able

Great patch!  Mitz and I discussed some changes on IRC that he's cooking up.

[Brady Eidson]

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.

[Brady Eidson]

Comment on attachment 12220 [details]
Make PageCaches resurrect-able

Do it! 

r+

[Brady Eidson]

Committed in r18588