Cookies are not available after turning off Private Browsing after the last window...
authorjberlin@webkit.org <jberlin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Sep 2011 00:57:43 +0000 (00:57 +0000)
committerjberlin@webkit.org <jberlin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Sep 2011 00:57:43 +0000 (00:57 +0000)
closed.
https://bugs.webkit.org/show_bug.cgi?id=67874

Reviewed by Darin Adler.

The private browsing storage session is a global setting that is being incorrectly set on a
per-page basis (see http://webkit.org/b/67870).

In this case, the global value was getting out of sync with the per-page setting:
1. The global value was getting set to true when setPrivateBrowsingEnabled(true) was called.
2. All Pages were then closed, destroying their Settings objects.
3. When a new Page was created, a new Settings object was created and its
   m_privateBrowsingEnabled value was getting set to false.
4. The WebPage settings were then applied to the new Settings object, resulting in
   setPrivateBrowsingEnabled(false) to be called.
5. An if (m_privateBrowsingEnabled == privateBrowsingEnabled) early return prevented the
   global value for the storage session from being destroyed.

* page/Settings.cpp:
(WebCore::Settings::setPrivateBrowsingEnabled):
Move the early return to be after setting the global private browsing values, and add a
clearer comment + FIXME.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@94892 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/Settings.cpp

index 2ad6fb8..359f121 100644 (file)
@@ -1,3 +1,29 @@
+2011-09-09  Jessie Berlin  <jberlin@apple.com>
+
+        Cookies are not available after turning off Private Browsing after the last window has been
+        closed.
+        https://bugs.webkit.org/show_bug.cgi?id=67874
+
+        Reviewed by Darin Adler.
+
+        The private browsing storage session is a global setting that is being incorrectly set on a
+        per-page basis (see http://webkit.org/b/67870).
+
+        In this case, the global value was getting out of sync with the per-page setting:
+        1. The global value was getting set to true when setPrivateBrowsingEnabled(true) was called.
+        2. All Pages were then closed, destroying their Settings objects.
+        3. When a new Page was created, a new Settings object was created and its
+           m_privateBrowsingEnabled value was getting set to false.
+        4. The WebPage settings were then applied to the new Settings object, resulting in
+           setPrivateBrowsingEnabled(false) to be called.
+        5. An if (m_privateBrowsingEnabled == privateBrowsingEnabled) early return prevented the
+           global value for the storage session from being destroyed.
+
+        * page/Settings.cpp:
+        (WebCore::Settings::setPrivateBrowsingEnabled):
+        Move the early return to be after setting the global private browsing values, and add a
+        clearer comment + FIXME.
+
 2011-09-09  Kentaro Hara  <haraken@google.com>
 
         Generate a WebKitCSSMatrix constructor of V8 using the IDL 'Constructor' extended attribute
index 2dc1fbb..9c70820 100644 (file)
@@ -412,16 +412,26 @@ void Settings::setSessionStorageQuota(unsigned sessionStorageQuota)
 
 void Settings::setPrivateBrowsingEnabled(bool privateBrowsingEnabled)
 {
-    if (m_privateBrowsingEnabled == privateBrowsingEnabled)
-        return;
-
+    // FIXME http://webkit.org/b/67870: The private browsing storage session and cookie private
+    // browsing mode (which is used if storage sessions are not available) are global settings, so
+    // it is misleading to have them as per-page settings.
+    // In addition, if they are treated as a per Page settings, the global values can get out of
+    // sync with the per Page value in the following situation:
+    // 1. The global values get set to true when setPrivateBrowsingEnabled(true) is called.
+    // 2. All Pages are closed, so all Settings objects go away.
+    // 3. A new Page is created, and a corresponding new Settings object is created - with
+    //    m_privateBrowsingEnabled starting out as false in the constructor.
+    // 4. The WebPage settings get applied to the new Page and setPrivateBrowsingEnabled(false)
+    //    is called, but an if (m_privateBrowsingEnabled == privateBrowsingEnabled) early return
+    //    prevents the global values from getting changed from true to false.
 #if USE(CFURLSTORAGESESSIONS)
     ResourceHandle::setPrivateBrowsingEnabled(privateBrowsingEnabled);
 #endif
-
-    // FIXME: We can only enable cookie private browsing mode globally, so it's misleading to have it as a per-page setting.
     setCookieStoragePrivateBrowsingEnabled(privateBrowsingEnabled);
 
+    if (m_privateBrowsingEnabled == privateBrowsingEnabled)
+        return;
+
     m_privateBrowsingEnabled = privateBrowsingEnabled;
     m_page->privateBrowsingStateChanged();
 }