WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660) 0 WebCore 0x31cfa010 WebCore::Page::sessionID() const + 0 (Page.cpp:1660) 1 WebCore 0x31565378 WebCore::DocumentLoader::stopLoading() + 364 (DocumentLoader.cpp:106) 2 WebCore 0x315717b2 WebCore::DocumentLoader::detachFromFrame() + 42 (DocumentLoader.cpp:919) 3 WebCore 0x315de0a6 WebCore::FrameLoader::detachViewsAndDocumentLoader() + 46 (FrameLoader.cpp:1680) 4 WebCore 0x315d91ae WebCore::CachedFrame::destroy() + 38 (CachedFrame.cpp:259) 5 WebCore 0x315d91ca WebCore::CachedFrame::destroy() + 66 (CachedFrame.cpp:263) 6 WebCore 0x315d9166 WebCore::CachedPage::~CachedPage() + 10 (CachedPage.cpp:74) 7 WebCore 0x31cfa890 WebCore::PageCache::prune(WebCore::PruningReason) + 40 (memory:2431) 8 WebCore 0x31cfa860 WebCore::PageCache::pruneToSizeNow(unsigned int, WebCore::PruningReason) + 12 (PageCache.cpp:356) 9 WebCore 0x3154fabc WebCore::FrameLoader::commitProvisionalLoad() + 204 (FrameLoader.cpp:1760) The DocumentLoader still thinks it is loading when the PageCache entry is being destroyed. We should never have a page that is loading in the PageCache so this should never happen. Radar: <rdar://problem/20613631>
Created attachment 253819 [details] Patch
Comment on attachment 253819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253819&action=review > Source/WebCore/ChangeLog:11 > + We sometimes crash when destroying a PageCache CachedFrame because its > + DocumentLoader is still loading. This should never happen as we are not > + supposed to let pages are still have pending loads into the PageCache. This seems like it could have a negative effect because pages with long running requests would never be cacheable (e.g. comet-style XMLHttpRequest). Also, will PingLoader also prevent going to page cache? I used to think that we stopped all loads when entering the page cache, but I cannot find that now through code inspection.
Comment on attachment 253819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253819&action=review >> Source/WebCore/ChangeLog:11 >> + supposed to let pages are still have pending loads into the PageCache. > > This seems like it could have a negative effect because pages with long running requests would never be cacheable (e.g. comet-style XMLHttpRequest). Also, will PingLoader also prevent going to page cache? > > I used to think that we stopped all loads when entering the page cache, but I cannot find that now through code inspection. I added layout tests for page-caching when there are pending XHRs and image loads a while back and they are still passing with this change. As explained below, we *do* stop pending loads when starting a new provisional load (see stopAllLoaders() call in FrameLoader::continueLoadAfterNavigationPolicy()). So *generally*, there are no pending loads at the time we call PageCache::canCache() / PageCache::add(). However, we don't stop JS (e.g. DOM timers) at this point so nothing prevents JS from starting new loads *after* stopAllLoaders() is called (when the provisional load is started) and *before* the provisional load is committed (which is when we put the page into PageCache and actually suspend all ActiveDOMObjects, including DOMTimers). Ideally, we would find a way to avoid prevent this so that we could PageCache more but short-term I want to fix this top crasher in a safe way.
Thank you, this makes sense.
Comment on attachment 253819 [details] Patch Alexey, do you agree with Chris’s justification? If so, could you review the patch?
Comment on attachment 253819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253819&action=review r+ with one change > Source/WebCore/page/DiagnosticLoggingKeys.h:63 > - static String loadingAPISenseKey(); > + static String isLoading(); isLoadingKey()
Committed r185017: <http://trac.webkit.org/changeset/185017>
Comment on attachment 253819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253819&action=review Thanks for reviewing. >> Source/WebCore/page/DiagnosticLoggingKeys.h:63 >> + static String isLoading(); > > isLoadingKey() Right, fixed before landing.