WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

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>

[Chris Dumez]

Created attachment 253819 [details]
Patch

[Alexey Proskuryakov]

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.

[Chris Dumez]

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.

[Alexey Proskuryakov]

Thank you, this makes sense.

[Darin Adler]

Comment on attachment 253819 [details]
Patch

Alexey, do you agree with Chris’s justification? If so, could you review the patch?

[Brady Eidson]

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

[Chris Dumez]

Committed r185017: <http://trac.webkit.org/changeset/185017>

[Chris Dumez]

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.