[webkit-changes] [186751] branches/safari-601.1-branch/Source/WebCore

Sun, 12 Jul 2015 23:46:16 -0700

Title: [186751] branches/safari-601.1-branch/Source/WebCore

Diff

Modified: branches/safari-601.1-branch/Source/WebCore/ChangeLog (186750 => 186751)


--- branches/safari-601.1-branch/Source/WebCore/ChangeLog	2015-07-13 06:44:22 UTC (rev 186750)
+++ branches/safari-601.1-branch/Source/WebCore/ChangeLog	2015-07-13 06:45:40 UTC (rev 186751)
@@ -1,5 +1,37 @@
 2015-07-12  Babak Shafiei  <bshaf...@apple.com>
 
+        Merge r186683.
+
+    2015-07-10  Brady Eidson  <beid...@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-12  Babak Shafiei  <bshaf...@apple.com>
+
         Merge r186718.
 
     2015-07-11  Joseph Pecoraro  <pecor...@apple.com>

Modified: branches/safari-601.1-branch/Source/WebCore/loader/FrameLoader.cpp (186750 => 186751)


--- branches/safari-601.1-branch/Source/WebCore/loader/FrameLoader.cpp	2015-07-13 06:44:22 UTC (rev 186750)
+++ branches/safari-601.1-branch/Source/WebCore/loader/FrameLoader.cpp	2015-07-13 06:45:40 UTC (rev 186751)
@@ -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);

Modified: branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.cpp (186750 => 186751)


--- branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.cpp	2015-07-13 06:44:22 UTC (rev 186750)
+++ branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.cpp	2015-07-13 06:45:40 UTC (rev 186751)
@@ -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;
 }

Modified: branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.h (186750 => 186751)


--- branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.h	2015-07-13 06:44:22 UTC (rev 186750)
+++ branches/safari-601.1-branch/Source/WebCore/loader/HistoryController.h	2015-07-13 06:45:40 UTC (rev 186751)
@@ -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*);

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to