Bug 145422 - WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
Summary: WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-27 16:39 PDT by Chris Dumez
Modified: 2015-06-07 17:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.65 KB, patch)
2015-05-27 17:00 PDT, Chris Dumez
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-05-27 16:39:05 PDT
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>
Comment 1 Chris Dumez 2015-05-27 17:00:14 PDT
Created attachment 253819 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-05-27 19:02:19 PDT
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 3 Chris Dumez 2015-05-27 19:28:40 PDT
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.
Comment 4 Alexey Proskuryakov 2015-05-27 20:30:03 PDT
Thank you, this makes sense.
Comment 5 Darin Adler 2015-05-29 15:38:40 PDT
Comment on attachment 253819 [details]
Patch

Alexey, do you agree with Chris’s justification? If so, could you review the patch?
Comment 6 Brady Eidson 2015-05-29 16:37:02 PDT
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()
Comment 7 Chris Dumez 2015-05-29 16:42:43 PDT
Committed r185017: <http://trac.webkit.org/changeset/185017>
Comment 8 Chris Dumez 2015-05-29 16:44:37 PDT
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.