Crash in HistoryController::updateForCommit dereferencing a null HistoryItem.
<rdar://problem/21371589> and https://bugs.webkit.org/show_bug.cgi?id=146842
Reviewed by Chris Dumez.
No new tests (Unknown how to reproduce).
This patch basically rolls back part of http://trac.webkit.org/changeset/179472.
r179472 changed HistoryController::setCurrentItem() to take a reference instead of a pointer.
Unfortunately, we sometimes call setCurrentItem(nullptr).
We'd like to *not* do that, and there are assertions in place to try to catch when we do,
but in the meantime it is not valid to dereference nullptr.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadSameDocumentItem):
* loader/HistoryController.cpp:
(WebCore::HistoryController::updateForCommit):
(WebCore::HistoryController::recursiveUpdateForCommit):
(WebCore::HistoryController::recursiveUpdateForSameDocumentNavigation):
(WebCore::HistoryController::setCurrentItem): Take a ptr instead of a ref.
(WebCore::HistoryController::createItem):
* loader/HistoryController.h:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@186683 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7cfaa98..b13cdd0 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2015-07-10 Brady Eidson <beidson@apple.com>
+
+ Crash in HistoryController::updateForCommit dereferencing a null HistoryItem.
+ <rdar://problem/21371589> and https://bugs.webkit.org/show_bug.cgi?id=146842
+
+ Reviewed by Chris Dumez.
+
+ No new tests (Unknown how to reproduce).
+
+ This patch basically rolls back part of http://trac.webkit.org/changeset/179472.
+
+ r179472 changed HistoryController::setCurrentItem() to take a reference instead of a pointer.
+ Unfortunately, we sometimes call setCurrentItem(nullptr).
+
+ We'd like to *not* do that, and there are assertions in place to try to catch when we do,
+ but in the meantime it is not valid to dereference nullptr.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::loadSameDocumentItem):
+
+ * loader/HistoryController.cpp:
+ (WebCore::HistoryController::updateForCommit):
+ (WebCore::HistoryController::recursiveUpdateForCommit):
+ (WebCore::HistoryController::recursiveUpdateForSameDocumentNavigation):
+ (WebCore::HistoryController::setCurrentItem): Take a ptr instead of a ref.
+ (WebCore::HistoryController::createItem):
+ * loader/HistoryController.h:
+
2015-07-10 Javier Fernandez <jfernandez@igalia.com>
[CSS Grid Layout] Grid item's auto-margins are not applied correctly
diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
index 901b676..b7201ee 100644
--- a/Source/WebCore/loader/FrameLoader.cpp
+++ b/Source/WebCore/loader/FrameLoader.cpp
@@ -3181,7 +3181,7 @@
if (FrameView* view = m_frame.view())
view->setWasScrolledByUser(false);
- history().setCurrentItem(item);
+ history().setCurrentItem(&item);
// loadInSameDocument() actually changes the URL and notifies load delegates of a "fake" load
loadInSameDocument(item.url(), item.stateObject(), false);
diff --git a/Source/WebCore/loader/HistoryController.cpp b/Source/WebCore/loader/HistoryController.cpp
index 454dd0a..d1020d8 100644
--- a/Source/WebCore/loader/HistoryController.cpp
+++ b/Source/WebCore/loader/HistoryController.cpp
@@ -470,8 +470,13 @@
// the provisional item for restoring state.
// Note previousItem must be set before we close the URL, which will
// happen when the data source is made non-provisional below
+
+ // FIXME: https://bugs.webkit.org/show_bug.cgi?id=146842
+ // We should always have a provisional item when committing, but we sometimes don't.
+ // Not having one leads to us not having a m_currentItem later, which is also a terrible known issue.
+ // We should get to the bottom of this.
ASSERT(m_provisionalItem);
- setCurrentItem(*m_provisionalItem);
+ setCurrentItem(m_provisionalItem.get());
m_provisionalItem = nullptr;
// Tell all other frames in the tree to commit their provisional items and
@@ -512,7 +517,7 @@
view->setWasScrolledByUser(false);
// Now commit the provisional item
- setCurrentItem(*m_provisionalItem);
+ setCurrentItem(m_provisionalItem.get());
m_provisionalItem = nullptr;
// Restore form state (works from currentItem)
@@ -561,7 +566,7 @@
return;
// Commit the provisional item.
- setCurrentItem(*m_provisionalItem);
+ setCurrentItem(m_provisionalItem.get());
m_provisionalItem = nullptr;
// Iterate over the rest of the tree.
@@ -577,11 +582,11 @@
m_frameLoadComplete = true;
}
-void HistoryController::setCurrentItem(HistoryItem& item)
+void HistoryController::setCurrentItem(HistoryItem* item)
{
m_frameLoadComplete = false;
m_previousItem = m_currentItem;
- m_currentItem = &item;
+ m_currentItem = item;
}
void HistoryController::setCurrentItemTitle(const StringWithDirection& title)
@@ -664,7 +669,7 @@
initializeItem(item);
// Set the item for which we will save document state
- setCurrentItem(item);
+ setCurrentItem(item.ptr());
return item;
}
diff --git a/Source/WebCore/loader/HistoryController.h b/Source/WebCore/loader/HistoryController.h
index 9d418fe..ef24521 100644
--- a/Source/WebCore/loader/HistoryController.h
+++ b/Source/WebCore/loader/HistoryController.h
@@ -74,7 +74,7 @@
void updateForFrameLoadCompleted();
HistoryItem* currentItem() const { return m_currentItem.get(); }
- void setCurrentItem(HistoryItem&);
+ void setCurrentItem(HistoryItem*);
void setCurrentItemTitle(const StringWithDirection&);
bool currentItemShouldBeReplaced() const;
WEBCORE_EXPORT void replaceCurrentItem(HistoryItem*);
