Crash in HistoryController::updateForCommit dereferencing a null HistoryItem.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Jul 2015 19:01:51 +0000 (19:01 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Jul 2015 19:01:51 +0000 (19:01 +0000)
<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: https://svn.webkit.org/repository/webkit/trunk@186683 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/HistoryController.cpp
Source/WebCore/loader/HistoryController.h

index 7cfaa98..b13cdd0 100644 (file)
@@ -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
index 901b676..b7201ee 100644 (file)
@@ -3181,7 +3181,7 @@ void FrameLoader::loadSameDocumentItem(HistoryItem& item)
     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);
index 454dd0a..d1020d8 100644 (file)
@@ -470,8 +470,13 @@ void HistoryController::updateForCommit()
         // 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 @@ void HistoryController::recursiveUpdateForCommit()
             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 @@ void HistoryController::recursiveUpdateForSameDocumentNavigation()
         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 @@ void HistoryController::updateForFrameLoadCompleted()
     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 @@ Ref<HistoryItem> HistoryController::createItem()
     initializeItem(item);
     
     // Set the item for which we will save document state
-    setCurrentItem(item);
+    setCurrentItem(item.ptr());
     
     return item;
 }
index 9d418fe..ef24521 100644 (file)
@@ -74,7 +74,7 @@ public:
     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*);