PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes:
Thread 6 name:  WebThread
Thread 6 Crashed:
0   WebCore                       	0x00000001941fb74c WebCore::PageCache::removeAllItemsForPage(WebCore::Page&) + 44 (CachedPage.h:45)
1   WebCore                       	0x00000001941f4e90 WebCore::Page::~Page() + 380 (Page.cpp:321)
2   WebCore                       	0x00000001941f4e90 WebCore::Page::~Page() + 380 (Page.cpp:321)
3   WebCore                       	0x0000000194513f1c WebCore::SVGImage::~SVGImage() + 64 (memory:2537)
4   WebCore                       	0x0000000194513f90 WebCore::SVGImage::~SVGImage() + 12 (SVGImage.cpp:72)
5   WebCore                       	0x00000001936e75a8 WebCore::CachedImage::~CachedImage() + 232 (RefCounted.h:145)
6   WebCore                       	0x00000001936e74b8 WebCore::CachedImage::~CachedImage() + 12 (CachedImage.cpp:82)
7   WebCore                       	0x000000019364b550 WebCore::CachedResource::unregisterHandle(WebCore::CachedResourceHandleBase*) + 184 (CachedResource.cpp:563)
8   WebCore                       	0x000000019364b488 WebCore::CachedResourceHandleBase::~CachedResourceHandleBase() + 32 (CachedResourceHandle.cpp:55)
9   WebCore                       	0x00000001936a511c WebCore::CachedResourceLoader::~CachedResourceLoader() + 428 (CachedResourceHandle.h:62)
10  WebCore                       	0x00000001936a4990 WebCore::Document::~Document() + 4120 (RefCounted.h:145)
11  WebCore                       	0x0000000193b681f4 WebCore::HTMLDocument::~HTMLDocument() + 12 (HTMLDocument.cpp:91)
12  WebCore                       	0x00000001936a375c WebCore::Node::~Node() + 172 (Document.h:325)
13  WebCore                       	0x000000019370b538 WebCore::HTMLSpanElement::~HTMLSpanElement() + 12 (HTMLElement.h:38)
14  WebCore                       	0x000000019369ed1c WebCore::Event::~Event() + 108 (EventTarget.h:65)
15  WebCore                       	0x000000019373cd34 WebCore::MouseEvent::~MouseEvent() + 104 (UIEventWithKeyState.h:31)
16  WebCore                       	0x00000001939d7358 WebCore::NavigationAction::~NavigationAction() + 144 (RefCounted.h:145)
17  WebCore                       	0x00000001936a00d4 WebCore::DocumentLoader::~DocumentLoader() + 1548 (NavigationAction.h:40)
18  WebKitLegacy                  	0x0000000194866d78 WebDocumentLoaderMac::~WebDocumentLoaderMac() + 84 (WebDocumentLoaderMac.h:40)
19  WebCore                       	0x00000001936f639c WebCore::CachedFrameBase::~CachedFrameBase() + 272 (RefCounted.h:145)
20  WebCore                       	0x00000001936f55b0 WebCore::CachedPage::~CachedPage() + 44 (CachedFrame.h:68)
21  WebCore                       	0x00000001941fb764 WebCore::PageCache::removeAllItemsForPage(WebCore::Page&) + 68 (memory:2537)
22  WebCore                       	0x00000001941f4e90 WebCore::Page::~Page() + 380 (Page.cpp:321)
23  WebKitLegacy                  	0x00000001949438cc WebKit::DeferredPageDestructor::tryDestruction() + 124 (memory:2537)
24  WebKitLegacy                  	0x0000000194939478 __29-[WebView(WebPrivate) _close]_block_invoke + 440 (WebView.mm:598)
25  WebCore                       	0x00000001946431d4 HandleRunSource(void*) + 704 (WebCoreThreadRun.cpp:99)
26  CoreFoundation                	0x000000018ea7542c __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (CFRunLoop.c:1943)
27  CoreFoundation                	0x000000018ea74d04 __CFRunLoopDoSources0 + 388 (CFRunLoop.c:2008)
28  CoreFoundation                	0x000000018ea729a8 __CFRunLoopRun + 744 (CFRunLoop.c:2821)
29  CoreFoundation                	0x000000018e9a2da4 CFRunLoopRunSpecific + 424 (CFRunLoop.c:3113)
30  WebCore                       	0x0000000193674608 RunWebThread(void*) + 456 (WebCoreThread.mm:694)
31  libsystem_pthread.dylib       	0x000000018db8968c _pthread_body + 240 (pthread.c:697)
32  libsystem_pthread.dylib       	0x000000018db8959c _pthread_start + 284 (pthread.c:744)
33  libsystem_pthread.dylib       	0x000000018db86cb4 thread_start + 4

This can happen when a Page containing an SVGImage is removed from PageCache and this results in the destruction of the SVGImage. Because the SVGImage has an internal utility Page, it will also call PageCache::removeAllItemsForPage(WebCore::Page&) upon destruction, causing us to reenter.

[Chris Dumez]

<rdar://problem/32177485>

[Chris Dumez]

Created attachment 315369 [details]
Patch

[Brady Eidson]

Comment on attachment 315369 [details]
Patch

Testable...? (Cause seems well understood)

[Chris Dumez]

(In reply to Brady Eidson from comment #3)
> Comment on attachment 315369 [details]
> Patch
> 
> Testable...? (Cause seems well understood)

Please read ChangeLog.

[Brady Eidson]

(In reply to Chris Dumez from comment #4)
> (In reply to Brady Eidson from comment #3)
> > Comment on attachment 315369 [details]
> > Patch
> > 
> > Testable...? (Cause seems well understood)
> 
> Please read ChangeLog.

Novel idea. I never read the ChangeLog before scanning for a test ;)

[Chris Dumez]

(In reply to Brady Eidson from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Brady Eidson from comment #3)
> > > Comment on attachment 315369 [details]
> > > Patch
> > > 
> > > Testable...? (Cause seems well understood)
> > 
> > Please read ChangeLog.
> 
> Novel idea. I never read the ChangeLog before scanning for a test ;)

Anyway, I tried hard and am open to ideas but getting the SVGImage to get destroyed at the right time is very tricky.

[WebKit Commit Bot]

Comment on attachment 315369 [details]
Patch

Clearing flags on attachment: 315369

Committed r219501: <http://trac.webkit.org/changeset/219501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.