WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
145422
WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
https://bugs.webkit.org/show_bug.cgi?id=145422
Summary
WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
Chris Dumez
Reported
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
>
Attachments
Patch
(5.65 KB, patch)
2015-05-27 17:00 PDT
,
Chris Dumez
beidson
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2015-05-27 17:00:14 PDT
Created
attachment 253819
[details]
Patch
Alexey Proskuryakov
Comment 2
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.
Chris Dumez
Comment 3
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.
Alexey Proskuryakov
Comment 4
2015-05-27 20:30:03 PDT
Thank you, this makes sense.
Darin Adler
Comment 5
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?
Brady Eidson
Comment 6
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()
Chris Dumez
Comment 7
2015-05-29 16:42:43 PDT
Committed
r185017
: <
http://trac.webkit.org/changeset/185017
>
Chris Dumez
Comment 8
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug