RESOLVED FIXED 174473
PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes 
https://bugs.webkit.org/show_bug.cgi?id=174473
Summary PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes
Chris Dumez
Reported 2017-07-13 13:24:21 PDT
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.
Attachments
Patch (4.33 KB, patch)
2017-07-13 13:38 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2017-07-13 13:24:40 PDT
<rdar://problem/32177485>
Chris Dumez
Comment 2 2017-07-13 13:38:12 PDT
Created attachment 315369 [details] Patch
Brady Eidson
Comment 3 2017-07-13 14:31:23 PDT
Comment on attachment 315369 [details] Patch Testable...? (Cause seems well understood)
Chris Dumez
Comment 4 2017-07-13 14:33:41 PDT
(In reply to Brady Eidson from comment #3) > Comment on attachment 315369 [details] > Patch > > Testable...? (Cause seems well understood) Please read ChangeLog.
Brady Eidson
Comment 5 2017-07-13 15:15:41 PDT
(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
Comment 6 2017-07-13 15:24:35 PDT
(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 7 2017-07-14 01:17:14 PDT
Comment on attachment 315369 [details] Patch Clearing flags on attachment: 315369 Committed r219501: <http://trac.webkit.org/changeset/219501>
WebKit Commit Bot
Comment 8 2017-07-14 01:17:15 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.