Changeset 211569 in webkit


Ignore:
Timestamp:
Feb 2, 2017 10:31:54 AM (7 years ago)
Author:
Chris Dumez
Message:

[Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
​https://bugs.webkit.org/show_bug.cgi?id=167738
<rdar://problem/30229990>

Reviewed by Andreas Kling.

Source/WebCore:

Upon destruction of a Page, we destroy the BackForwardClient, which is supposed
to keep track of HistoryItems associated to this particular page and remove them
from the PageCache. Given the crash trace, the issue seems to be that some
HistoryItems associated with the Page sometimes linger in the PageCache *after*
the Page has been destroyed, which leads to crashes later on when pruning the
PageCache.

In order to make the process more robust, this patch refactors the code so that
the Page is now in charge of removing all its associated HistoryItems from the
PageCache instead of relying on the BackForwardClient. Also, instead of having
the Page keep track of which HistoryItems are associated with it (which is
error prone), we now scan all PageCache entries instead to find which ones are
associated with the Page. While this is in theory slower, this is much safer
and in practice not an issue because the PageCache usually has 3-5 entries.

No new tests, could not reproduce.

  • history/CachedPage.cpp:

(WebCore::CachedPage::CachedPage):

  • history/CachedPage.h:

(WebCore::CachedPage::page):

  • history/PageCache.cpp:

(WebCore::PageCache::removeAllItemsForPage):

  • history/PageCache.h:
  • page/Page.cpp:

(WebCore::Page::~Page):

Source/WebKit/mac:

The BackForwardClient no longer needs to worry about removing HistoryItems
from the PageCache now that WebCore takes care of it.

  • History/BackForwardList.mm:

(BackForwardList::close):

Source/WebKit/win:

The BackForwardClient no longer needs to worry about removing HistoryItems
from the PageCache now that WebCore takes care of it.

  • BackForwardList.cpp:

(BackForwardList::close):

Source/WebKit2:

The BackForwardClient no longer needs to worry about removing HistoryItems
from the PageCache now that WebCore takes care of it.

  • WebProcess/WebPage/WebBackForwardListProxy.cpp:

(WebKit::WebBackForwardListProxy::addItemFromUIProcess):
(WebKit::WebBackForwardListProxy::addItem):
(WebKit::WebBackForwardListProxy::close):

  • WebProcess/WebPage/WebBackForwardListProxy.h:
Location:
trunk/Source
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r211564 r211569  
     12017-02-02  Chris Dumez  <cdumez@apple.com>
     2
     3        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
     4        https://bugs.webkit.org/show_bug.cgi?id=167738
     5        <rdar://problem/30229990>
     6
     7        Reviewed by Andreas Kling.
     8
     9        Upon destruction of a Page, we destroy the BackForwardClient, which is supposed
     10        to keep track of HistoryItems associated to this particular page and remove them
     11        from the PageCache. Given the crash trace, the issue seems to be that some
     12        HistoryItems associated with the Page sometimes linger in the PageCache *after*
     13        the Page has been destroyed, which leads to crashes later on when pruning the
     14        PageCache.
     15
     16        In order to make the process more robust, this patch refactors the code so that
     17        the Page is now in charge of removing all its associated HistoryItems from the
     18        PageCache instead of relying on the BackForwardClient. Also, instead of having
     19        the Page keep track of which HistoryItems are associated with it (which is
     20        error prone), we now scan all PageCache entries instead to find which ones are
     21        associated with the Page. While this is in theory slower, this is much safer
     22        and in practice not an issue because the PageCache usually has 3-5 entries.
     23
     24        No new tests, could not reproduce.
     25
     26        * history/CachedPage.cpp:
     27        (WebCore::CachedPage::CachedPage):
     28        * history/CachedPage.h:
     29        (WebCore::CachedPage::page):
     30        * history/PageCache.cpp:
     31        (WebCore::PageCache::removeAllItemsForPage):
     32        * history/PageCache.h:
     33        * page/Page.cpp:
     34        (WebCore::Page::~Page):
     35
    1362017-02-02  Antti Koivisto  <antti@apple.com>
    237

  • trunk/Source/WebCore/history/CachedPage.cpp

    r207620 r211569  
    5151
    5252CachedPage::CachedPage(Page& page)
    53     : m_expirationTime(monotonicallyIncreasingTime() + page.settings().backForwardCacheExpirationInterval())
     53    : m_page(page)
     54    , m_expirationTime(monotonicallyIncreasingTime() + page.settings().backForwardCacheExpirationInterval())
    5455    , m_cachedMainFrame(std::make_unique<CachedFrame>(page.mainFrame()))
    5556{

  • trunk/Source/WebCore/history/CachedPage.h

    r208646 r211569  
    4343    void clear();
    4444
     45    Page& page() const { return m_page; }
    4546    Document* document() const { return m_cachedMainFrame->document(); }
    4647    DocumentLoader* documentLoader() const { return m_cachedMainFrame->documentLoader(); }
    … …  
    5960
    6061private:
     62    Page& m_page;
    6163    double m_expirationTime;
    6264    std::unique_ptr<CachedFrame> m_cachedMainFrame;

  • trunk/Source/WebCore/history/PageCache.cpp

    r211291 r211569  
    467467}
    468468
     469void PageCache::removeAllItemsForPage(Page& page)
     470{
     471    for (auto it = m_items.begin(); it != m_items.end();) {
     472        // Increment iterator first so it stays invalid after the removal.
     473        auto current = it;
     474        ++it;
     475        if (&(*current)->m_cachedPage->page() == &page) {
     476            (*current)->m_cachedPage = nullptr;
     477            m_items.remove(current);
     478        }
     479    }
     480}
     481
    469482CachedPage* PageCache::get(HistoryItem& item, Page* page)
    470483{

  • trunk/Source/WebCore/history/PageCache.h

    r209865 r211569  
    5858    std::unique_ptr<CachedPage> take(HistoryItem&, Page*);
    5959
     60    void removeAllItemsForPage(Page&);
     61
    6062    unsigned pageCount() const { return m_items.size(); }
    6163    WEBCORE_EXPORT unsigned frameCount() const;

  • trunk/Source/WebCore/page/Page.cpp

    r211254 r211569  
    326326
    327327    backForward().close();
     328    PageCache::singleton().removeAllItemsForPage(*this);
    328329
    329330#ifndef NDEBUG

  • trunk/Source/WebKit/mac/ChangeLog

    r211551 r211569  
     12017-02-02  Chris Dumez  <cdumez@apple.com>
     2
     3        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
     4        https://bugs.webkit.org/show_bug.cgi?id=167738
     5        <rdar://problem/30229990>
     6
     7        Reviewed by Andreas Kling.
     8
     9        The BackForwardClient no longer needs to worry about removing HistoryItems
     10        from the PageCache now that WebCore takes care of it.
     11
     12        * History/BackForwardList.mm:
     13        (BackForwardList::close):
     14
    1152017-02-02  Yongjun Zhang  <yongjun_zhang@apple.com>
    216

  • trunk/Source/WebKit/mac/History/BackForwardList.mm

    r207419 r211569  
    226226void BackForwardList::close()
    227227{
    228     for (auto& item : m_entries)
    229         PageCache::singleton().remove(item);
    230228    m_entries.clear();
    231229    m_entryHash.clear();

  • trunk/Source/WebKit/win/BackForwardList.cpp

    r210697 r211569  
    226226void BackForwardList::close()
    227227{
    228     for (auto& item : m_entries)
    229         PageCache::singleton().remove(item);
    230228    m_entries.clear();
    231229    m_entryHash.clear();

  • trunk/Source/WebKit/win/ChangeLog

    r211341 r211569  
     12017-02-02  Chris Dumez  <cdumez@apple.com>
     2
     3        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
     4        https://bugs.webkit.org/show_bug.cgi?id=167738
     5        <rdar://problem/30229990>
     6
     7        Reviewed by Andreas Kling.
     8
     9        The BackForwardClient no longer needs to worry about removing HistoryItems
     10        from the PageCache now that WebCore takes care of it.
     11
     12        * BackForwardList.cpp:
     13        (BackForwardList::close):
     14
    1152017-01-28  Yoav Weiss  <yoav@yoav.ws>
    216

  • trunk/Source/WebKit2/ChangeLog

    r211568 r211569  
     12017-02-02  Chris Dumez  <cdumez@apple.com>
     2
     3        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
     4        https://bugs.webkit.org/show_bug.cgi?id=167738
     5        <rdar://problem/30229990>
     6
     7        Reviewed by Andreas Kling.
     8
     9        The BackForwardClient no longer needs to worry about removing HistoryItems
     10        from the PageCache now that WebCore takes care of it.
     11
     12        * WebProcess/WebPage/WebBackForwardListProxy.cpp:
     13        (WebKit::WebBackForwardListProxy::addItemFromUIProcess):
     14        (WebKit::WebBackForwardListProxy::addItem):
     15        (WebKit::WebBackForwardListProxy::close):
     16        * WebProcess/WebPage/WebBackForwardListProxy.h:
     17
    1182017-02-02  Anders Carlsson  <andersca@apple.com>
    219

  • trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp

    r210666 r211569  
    101101    ASSERT(!idToHistoryItemMap().contains(itemID));
    102102
    103     m_associatedItemIDs.add(itemID);
    104 
    105103    historyItemToIDMap().set<ItemAndPageID>(item.ptr(), { .itemID = itemID, .pageID = pageID });
    106104    idToHistoryItemMap().set(itemID, item.ptr());
    … …  
    155153    ASSERT(!idToHistoryItemMap().contains(itemID));
    156154
    157     m_associatedItemIDs.add(itemID);
    158 
    159155    historyItemToIDMap().set<ItemAndPageID>(item.ptr(), { .itemID = itemID, .pageID = m_page->pageID() });
    160156    idToHistoryItemMap().set(itemID, item.ptr());
    … …  
    215211void WebBackForwardListProxy::close()
    216212{
    217     for (auto& itemID : m_associatedItemIDs) {
    218         if (HistoryItem* item = itemForID(itemID))
    219             WebCore::PageCache::singleton().remove(*item);
    220     }
    221 
    222     m_associatedItemIDs.clear();
    223213    m_page = nullptr;
    224214}

  • trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h

    r210666 r211569  
    6161
    6262    WebPage* m_page;
    63     HashSet<uint64_t> m_associatedItemIDs;
    6463};
    6564
Note: See TracChangeset for help on using the changeset viewer.