Modified: branches/safari-536-branch/Source/WebKit2/ChangeLog (119036 => 119037)
--- branches/safari-536-branch/Source/WebKit2/ChangeLog 2012-05-31 03:22:19 UTC (rev 119036)
+++ branches/safari-536-branch/Source/WebKit2/ChangeLog 2012-05-31 03:23:54 UTC (rev 119037)
@@ -1,5 +1,31 @@
2012-05-30 Lucas Forschler <[email protected]>
+ Merge 118441
+
+ 2012-05-24 Brady Eidson <[email protected]>
+
+ <rdar://problem/10090764> and https://bugs.webkit.org/show_bug.cgi?id=87417
+ (Unrepro) Crashes saving session state in WebBackForwardList
+
+ Reviewed by Darin Adler.
+
+ * UIProcess/WebBackForwardList.cpp:
+ (WebKit::WebBackForwardList::addItem): Null check the proposed item and also m_page, to make
+ sure the page hasn't been closed making this list inactive. Be more aggressive about
+ clearing the current entries out if there is no current item index.
+ (WebKit::WebBackForwardList::itemAtIndex): Early null return if there is no current index.
+ (WebKit::WebBackForwardList::clear): Don't put the current item back in the array if there was
+ no current item.
+
+ * UIProcess/cf/WebBackForwardListCF.cpp:
+ (WebKit::WebBackForwardList::createCFDictionaryRepresentation): Don't create a meaningless WebURL.
+ Don't successfully return a dictionary if any of the entries were null. Be more aggressive about
+ validating the current index we plan to return in the dictionary.
+ (WebKit::WebBackForwardList::restoreFromCFDictionaryRepresentation): More aggressively validate the
+ current index read from disk. Replace a meaningless sanity check with our typical ASSERT.
+
+2012-05-30 Lucas Forschler <[email protected]>
+
Merge 118439
2012-05-24 Anders Carlsson <[email protected]>
Modified: branches/safari-536-branch/Source/WebKit2/UIProcess/WebBackForwardList.cpp (119036 => 119037)
--- branches/safari-536-branch/Source/WebKit2/UIProcess/WebBackForwardList.cpp 2012-05-31 03:22:19 UTC (rev 119036)
+++ branches/safari-536-branch/Source/WebKit2/UIProcess/WebBackForwardList.cpp 2012-05-31 03:23:54 UTC (rev 119037)
@@ -39,7 +39,7 @@
, m_closed(true)
, m_enabled(true)
{
- ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size());
+ ASSERT(m_page);
}
WebBackForwardList::~WebBackForwardList()
@@ -61,40 +61,51 @@
{
ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size());
- if (m_capacity == 0 || !m_enabled)
+ if (!m_capacity || !m_enabled || !newItem || !m_page)
return;
Vector<RefPtr<APIObject> > removedItems;
- // Toss anything in the forward list
if (m_current != NoCurrentItemIndex) {
+ // Toss everything in the forward list.
unsigned targetSize = m_current + 1;
removedItems.reserveCapacity(m_entries.size() - targetSize);
while (m_entries.size() > targetSize) {
- if (m_page)
- m_page->backForwardRemovedItem(m_entries.last()->itemID());
+ m_page->backForwardRemovedItem(m_entries.last()->itemID());
removedItems.append(m_entries.last().release());
m_entries.removeLast();
}
- }
- // Toss the first item if the list is getting too big, as long as we're not using it
- // (or even if we are, if we only want 1 entry).
- if (m_entries.size() == m_capacity && (m_current != 0 || m_capacity == 1)) {
- if (m_page)
+ // Toss the first item if the list is getting too big, as long as we're not using it
+ // (or even if we are, if we only want 1 entry).
+ if (m_entries.size() == m_capacity && (m_current || m_capacity == 1)) {
m_page->backForwardRemovedItem(m_entries[0]->itemID());
- removedItems.append(m_entries[0].release());
- m_entries.remove(0);
- m_current--;
+ removedItems.append(m_entries[0].release());
+ m_entries.remove(0);
+ m_current--;
+ }
+ } else {
+ // If we have no current item index, we should have no other entries before adding this new item.
+ size_t size = m_entries.size();
+ for (size_t i = 0; i < size; ++i) {
+ m_page->backForwardRemovedItem(m_entries[i]->itemID());
+ removedItems.append(m_entries[i].release());
+ }
+ m_entries.clear();
}
+
+ if (m_current == NoCurrentItemIndex)
+ m_current = 0;
+ else
+ m_current++;
- m_entries.insert(m_current + 1, newItem);
- m_current++;
+ // m_current never be pointing more than 1 past the end of the entries Vector.
+ // If it is, something has gone wrong and we should not try to insert the new item.
+ ASSERT(m_current <= m_entries.size());
+ if (m_current <= m_entries.size())
+ m_entries.insert(m_current, newItem);
- if (m_page)
- m_page->didChangeBackForwardList(newItem, &removedItems);
-
- ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size());
+ m_page->didChangeBackForwardList(newItem, &removedItems);
}
void WebBackForwardList::goToItem(WebBackForwardListItem* item)
@@ -147,8 +158,11 @@
{
ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size());
+ if (m_current == NoCurrentItemIndex)
+ return 0;
+
// Do range checks without doing math on index to avoid overflow.
- if (index < -static_cast<int>(m_current))
+ if (index < -backListCount())
return 0;
if (index > forwardListCount())
@@ -232,12 +246,17 @@
if (i != m_current)
removedItems.append(m_entries[i].release());
}
-
- m_entries.shrink(1);
- m_entries[0] = currentItem.release();
m_current = 0;
+ if (currentItem) {
+ m_entries.shrink(1);
+ m_entries[0] = currentItem.release();
+ } else {
+ m_entries.clear();
+ m_current = NoCurrentItemIndex;
+ }
+
if (m_page)
m_page->didChangeBackForwardList(0, &removedItems);
}
Modified: branches/safari-536-branch/Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp (119036 => 119037)
--- branches/safari-536-branch/Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp 2012-05-31 03:22:19 UTC (rev 119036)
+++ branches/safari-536-branch/Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp 2012-05-31 03:23:54 UTC (rev 119037)
@@ -58,10 +58,14 @@
RetainPtr<CFMutableArrayRef> entries(AdoptCF, CFArrayCreateMutable(0, m_entries.size(), &kCFTypeArrayCallBacks));
// We may need to update the current index to account for entries that are filtered by the callback.
- int currentIndex = m_current;
+ CFIndex currentIndex = m_current;
for (size_t i = 0; i < m_entries.size(); ++i) {
- RefPtr<WebURL> webURL = WebURL::create(m_entries[i]->url());
+ // If we somehow ended up with a null entry then we should consider the data invalid and not save session history at all.
+ ASSERT(m_entries[i]);
+ if (!m_entries[i])
+ return 0;
+
if (filter && !filter(toAPI(m_page), WKPageGetSessionHistoryURLValueType(), toURLRef(m_entries[i]->originalURL().impl()), context)) {
if (i <= static_cast<size_t>(m_current))
currentIndex--;
@@ -82,10 +86,19 @@
RetainPtr<CFDictionaryRef> entryDictionary(AdoptCF, CFDictionaryCreate(0, keys, values, 4, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
CFArrayAppendValue(entries.get(), entryDictionary.get());
}
+
+ // If all items before and including the current item were filtered then currentIndex will be -1.
+ // Assuming we didn't start out with NoCurrentItemIndex, we should store "current" to point at the first item.
+ if (currentIndex == -1 && m_current != NoCurrentItemIndex && CFArrayGetCount(entries.get()))
+ currentIndex = 0;
- ASSERT(currentIndex < CFArrayGetCount(entries.get()) && currentIndex >= static_cast<int>(NoCurrentItemIndex));
- RetainPtr<CFNumberRef> currentIndexNumber(AdoptCF, CFNumberCreate(0, kCFNumberIntType, ¤tIndex));
+ // FIXME: We're relying on currentIndex == -1 to mean the exact same thing as NoCurrentItemIndex (UINT_MAX) in unsigned form.
+ // That seems implicit and fragile and we should find a better way of representing the NoCurrentItemIndex case.
+ if (m_current == NoCurrentItemIndex || !CFArrayGetCount(entries.get()))
+ currentIndex = -1;
+ RetainPtr<CFNumberRef> currentIndexNumber(AdoptCF, CFNumberCreate(0, kCFNumberCFIndexType, ¤tIndex));
+
const void* keys[2] = { SessionHistoryCurrentIndexKey(), SessionHistoryEntriesKey() };
const void* values[2] = { currentIndexNumber.get(), entries.get() };
@@ -100,11 +113,16 @@
return false;
}
- CFIndex currentIndex;
- if (!CFNumberGetValue(cfIndex, kCFNumberCFIndexType, ¤tIndex)) {
+ CFIndex currentCFIndex;
+ if (!CFNumberGetValue(cfIndex, kCFNumberCFIndexType, ¤tCFIndex)) {
LOG(SessionState, "WebBackForwardList dictionary representation does not have a valid integer current index");
return false;
}
+
+ if (currentCFIndex < -1) {
+ LOG(SessionState, "WebBackForwardList dictionary representation contains a negative current index that is bogus (%ld)", currentCFIndex);
+ return false;
+ }
CFArrayRef cfEntries = (CFArrayRef)CFDictionaryGetValue(dictionary, SessionHistoryEntriesKey());
if (!cfEntries || CFGetTypeID(cfEntries) != CFArrayGetTypeID()) {
@@ -113,12 +131,16 @@
}
CFIndex size = CFArrayGetCount(cfEntries);
- if (currentIndex != static_cast<CFIndex>(NoCurrentItemIndex) && currentIndex >= size) {
- LOG(SessionState, "WebBackForwardList dictionary representation contains an invalid current index (%ld) for the number of entries (%ld)", currentIndex, size);
+ if (currentCFIndex != -1 && currentCFIndex >= size) {
+ LOG(SessionState, "WebBackForwardList dictionary representation contains an invalid current index (%ld) for the number of entries (%ld)", currentCFIndex, size);
return false;
}
- if (currentIndex == static_cast<CFIndex>(NoCurrentItemIndex) && size) {
+ // FIXME: We're relying on currentIndex == -1 to mean the exact same thing as NoCurrentItemIndex (UINT_MAX) in unsigned form.
+ // That seems implicit and fragile and we should find a better way of representing the NoCurrentItemIndex case.
+ uint32_t currentIndex = currentCFIndex == -1 ? NoCurrentItemIndex : currentCFIndex;
+
+ if (currentIndex == NoCurrentItemIndex && size) {
LOG(SessionState, "WebBackForwardList dictionary representation says there is no current item index, but there is a list of %ld entries - this is bogus", size);
return false;
}
