Closed Bug 679957 Opened 13 years ago Closed 11 years ago

Private Browsing + Active File Downloading + Quit = Useless Warning

Categories

(Firefox :: Private Browsing, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: temhawk, Assigned: rwgmez)

References

Details

(Whiteboard: [mentor=jdm][lang=js][lang=c++])

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/534.50 (KHTML, like Gecko) Version/5.1 Safari/534.50

Steps to reproduce:

Start Private Browsing session.

Start a file download.

Tell Firefox to quit (⌘Q) while the file is still being downloaded.


Actual results:

A warning message appeared (on top of the Downloads window) giving me the option to either "Stay in Private Browsing Mode" or "Cancel 1 Download".

I did not want to cancel the download and lose the progress that it had made (it was a fairly big file) so I clicked the first option.

Firefox quit anyway and my download was lost!


Expected results:

Firefox should have not quit and thereby force me to restart a lengthy download.
WFM:
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1
Reproducible for me on
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110817 Firefox/9.0a1
and also on
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110817 Firefox/9.0a1

The same behavior is present in
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

I have tried to reproduce this issue also on Firefox 3.6.19 and on 3.6.20 and the browser closes without any warning message. 

Setting resolution to NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #554549 - Flags: review?(gavin.sharp)
Gavin: ping?
Gavin: ping again...
Bah, Gavin is not CCed on this bug :(
Here is the reason behind this bug.  When Firefox is about to quit, we leave the private browsing mode (in response to quit-application-granted).  While doing that, we call _canLeavePrivateBrowsingMode, which confirms with the user whether it's OK to leave the PB mode.  If the user says "no", we cancel the PB mode transition, but we still go ahead and quit the application.

What this fix does is that it makes the PB service also handle quit-application-requested.  In response to that event, it checks whether we can leave the PB mode by prompting the user, and if that happens, it aborts shutdown.  Now, if the user says "it's OK to leave the PB mode", we respond to quit-appliction-granted, and there we confirm the transition again, so the user would see the prompt again even though they've said "it's OK to leave".  This is what the _confirmedPBModeChange flag is supposed to prevent.

Gavin brought up a good point that the flag is never set back to false.  But I'm not sure when we should set it to false.  We only want to set it back to false if _any_ of the quit-application-requested observers cancel the shutdown process, but there is no notification for that, as far as I know.
Comment on attachment 554549 [details] [diff] [review]
Patch (v1)

Need a solution to the last paragraph of comment 8
Attachment #554549 - Flags: review?(gavin.sharp) → review-
Do you have any suggestions?  I'm out of ideas...
Could we just add a quit-application-cancelled notification, and work in a fix for bug 720215 as well?
(In reply to Josh Matthews [:jdm] from comment #11)
> Could we just add a quit-application-cancelled notification, and work in a
> fix for bug 720215 as well?

Sounds promising!
I doubt Ehsan will contest with my reading of reality by unassigning him. We have a proposed fix: add a quit-application-cancelled notification that is fired from the same place as quit-application-requested, and modify Ehsan's patch to observe it and reset the new flag.
Assignee: ehsan → nobody
Whiteboard: [mentor=jdm][lang=js][lang=c++]
Rich, please see comment 11 for a description of what needs to be done here.  Thanks!
Comment on attachment 625335 [details] [diff] [review]
A modification to Ehsan's patch to make no extra warning appear when quitting

># HG changeset patch
># Parent d3621795d5fd27b13b32427e1d311042bdf425d7
># User Ehsan Akhgari <ehsan@mozilla.com>, Rich Gomez <rwgmez@gmail.com>
>Bug 679957 - Make the private browsing service correctly confirm whether it should leave the private browsing mode if the user chooses to quit the application while the private browsing mode is active
>
>diff -r 895e12563245 browser/base/content/aboutDialog.js
>--- a/browser/base/content/aboutDialog.js	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/base/content/aboutDialog.js	Fri May 18 19:07:23 2012 -0700
>@@ -263,7 +263,10 @@
> 
>       // Something aborted the quit process.
>       if (cancelQuit.data)
>+	  {
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>         return;
>+	  }
> 
>       let appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].
>                        getService(Components.interfaces.nsIAppStartup);
>diff -r 895e12563245 browser/base/content/browser.js
>--- a/browser/base/content/browser.js	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/base/content/browser.js	Fri May 18 19:07:23 2012 -0700
>@@ -7465,7 +7465,10 @@
> 
>       // Something aborted the quit process.
>       if (cancelQuit.data)
>+	  {
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", null);
>         return;
>+	  }
> 
>       let as = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>       as.quit(Ci.nsIAppStartup.eRestarti386 | Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>diff -r 895e12563245 browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>--- a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js	Fri May 18 19:07:23 2012 -0700
>@@ -91,6 +91,8 @@
>   this._obs = Cc["@mozilla.org/observer-service;1"].
>               getService(Ci.nsIObserverService);
>   this._obs.addObserver(this, "profile-after-change", true);
>+  this._obs.addObserver(this, "quit-application-requested", true);
>+  this._obs.addObserver(this, "quit-application-cancelled", true);
>   this._obs.addObserver(this, "quit-application-granted", true);
>   this._obs.addObserver(this, "private-browsing", true);
>   this._obs.addObserver(this, "command-line-startup", true);
>@@ -133,6 +135,9 @@
> 
>   // Whether private browsing has been turned on from the command line
>   _lastChangedByCommandLine: false,
>+  
>+  // Whether the PB mode change has already been confirmed
>+  _confirmedPBModeChange: false,
> 
>   // Telemetry measurements
>   _enterTimestamps: {},
>@@ -483,6 +488,20 @@
>         }
>         this._obs.removeObserver(this, "profile-after-change");
>         break;
>+	  case "quit-application-requested":
>+        // Prevent the application from quitting if the user requests to stay in
>+        // private browsing mode.
>+        if (this.privateBrowsingEnabled && !this._canLeavePrivateBrowsingMode()) {
>+          aSubject.QueryInterface(Ci.nsISupportsPRBool);
>+          aSubject.data = true;
>+          this._confirmedPBModeChange = true;
>+        }
>+	    break;
>+	  case "quit-application-cancelled":
>+	    // Change the flag so the application does not prompt the user again if 
>+		// they request to quit private browsing mode.
>+		this._confirmedPBModeChange = false;
>+	    break;
>       case "quit-application-granted":
>         this._unload();
>         break;
>@@ -586,14 +605,16 @@
>       return;
> 
>     try {
>-      if (val) {
>-        if (!this._canEnterPrivateBrowsingMode())
>-          return;
>-      }
>-      else {
>-        if (!this._canLeavePrivateBrowsingMode())
>-          return;
>-      }
>+      if (this._confirmedPBModeChange) {
>+        if (val) {
>+          if (!this._canEnterPrivateBrowsingMode())
>+            return;
>+        }
>+        else {
>+          if (!this._canLeavePrivateBrowsingMode())
>+            return;
>+        }
>+	  }
> 
>       this._ensureCanCloseWindows();
> 
>diff -r 895e12563245 browser/components/privatebrowsing/test/unit/do_test_privatebrowsing_exit_cancel.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/privatebrowsing/test/unit/do_test_privatebrowsing_exit_cancel.js	Fri May 18 19:29:51 2012 -0700
>@@ -0,0 +1,77 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Private Browsing Tests.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ehsan Akhgari <ehsan.akhgari@gmail.com> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// This test makes sure that the canceling the PB mode when quitting can
>+// correctly prevent the application from exiting.
>+
>+function do_test() {
>+  // initialization
>+  var os = Cc["@mozilla.org/observer-service;1"].
>+           getService(Ci.nsIObserverService);
>+  var pb = Cc[PRIVATEBROWSING_CONTRACT_ID].
>+           getService(Ci.nsIPrivateBrowsingService);
>+
>+  var expectedQuitting;
>+  var called = 0;
>+  var observer = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic == kPrivateBrowsingCancelVoteNotification &&
>+          aData == kExit) {
>+        do_check_neq(aSubject, null);
>+        try {
>+          aSubject.QueryInterface(Ci.nsISupportsPRBool);
>+        } catch (ex) {
>+          do_throw("aSubject was not null, but wasn't an nsISupportsPRBool");
>+        }
>+        aSubject.data = true;
>+      }
>+    }
>+  };
>+
>+  // set the observer
>+  os.addObserver(observer, kPrivateBrowsingCancelVoteNotification, false);
>+  os.addObserver(observer, "quit-application-granted", false);
>+
>+  // enter the private browsing mode
>+  pb.privateBrowsingEnabled = true;
>+
>+  // Simulate an exit
>+  var data = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
>+  data.data = false;
>+  os.notifyObservers(data, "quit-application-requested", null);
>+  do_check_true(data.data);
>+}
>diff -r 895e12563245 browser/components/privatebrowsing/test/unit/test_privatebrowsing_exit_cancel.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/privatebrowsing/test/unit/test_privatebrowsing_exit_cancel.js	Fri May 18 19:29:51 2012 -0700
>@@ -0,0 +1,45 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Private Browsing Tests.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ehsan Akhgari <ehsan.akhgari@gmail.com> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// This test makes sure that the canceling the PB mode when quitting can
>+// correctly prevent the application from exiting.
>+
>+function run_test() {
>+  PRIVATEBROWSING_CONTRACT_ID = "@mozilla.org/privatebrowsing;1";
>+  load("do_test_privatebrowsing_exit_cancel.js");
>+  do_test();
>+}
>diff -r 895e12563245 browser/components/privatebrowsing/test/unit/test_privatebrowsingwrapper_exit_cancel.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/privatebrowsing/test/unit/test_privatebrowsingwrapper_exit_cancel.js	Fri May 18 19:29:51 2012 -0700
>@@ -0,0 +1,46 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Private Browsing Tests.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ehsan Akhgari <ehsan.akhgari@gmail.com> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// This test makes sure that the canceling the PB mode when quitting can
>+// correctly prevent the application from exiting.
>+
>+function run_test() {
>+  PRIVATEBROWSING_CONTRACT_ID = "@mozilla.org/privatebrowsing-wrapper;1";
>+  load("do_test_privatebrowsing_exit_cancel.js");
>+  do_test();
>+}
>+
>diff -r 895e12563245 browser/components/privatebrowsing/test/unit/xpcshell.ini
>--- a/browser/components/privatebrowsing/test/unit/xpcshell.ini	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/components/privatebrowsing/test/unit/xpcshell.ini	Fri May 18 19:07:23 2012 -0700
>@@ -12,10 +12,12 @@
> [test_privatebrowsing_autostart.js]
> [test_privatebrowsing_commandline.js]
> [test_privatebrowsing_exit.js]
>+[test_privatebrowsing_exit_cancel.js]
> [test_privatebrowsing_telemetry.js]
> [test_privatebrowsingwrapper_autostart.js]
> [test_privatebrowsingwrapper_commandline.js]
> [test_privatebrowsingwrapper_exit.js]
>+[test_privatebrowsingwrapper_exit_cancel.js]
> [test_privatebrowsingwrapper_placesTitleNoUpdate.js]
> [test_privatebrowsingwrapper_removeDataFromDomain.js]
> [test_privatebrowsingwrapper_removeDataFromDomain_activeDownloads.js]
>diff -r 895e12563245 browser/components/sessionstore/src/nsSessionStore.js
>--- a/browser/components/sessionstore/src/nsSessionStore.js	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/components/sessionstore/src/nsSessionStore.js	Fri May 18 19:07:23 2012 -0700
>@@ -491,7 +491,7 @@
>         break;
>       case "quit-application-requested":
>         this.onQuitApplicationRequested();
>-        break;
>+        break;  
>       case "quit-application-granted":
>         this.onQuitApplicationGranted();
>         break;
>diff -r 895e12563245 browser/devtools/debugger/debugger-controller.js
>--- a/browser/devtools/debugger/debugger-controller.js	Thu May 17 14:01:53 2012 -0400
>+++ b/browser/devtools/debugger/debugger-controller.js	Fri May 18 19:07:23 2012 -0700
>@@ -294,6 +294,7 @@
> 
>     // Somebody canceled our quit request.
>     if (canceled.data) {
>+	  Services.obs.notifyObservers(canceled, "quit-application-cancelled", null);
>       return;
>     }
> 
>diff -r 895e12563245 build/bloatcycle.html
>--- a/build/bloatcycle.html	Thu May 17 14:01:53 2012 -0400
>+++ b/build/bloatcycle.html	Fri May 18 19:07:23 2012 -0700
>@@ -72,6 +72,7 @@
>     // Something aborted the quit process. 
>     if (cancelQuit.data)
>     {
>+	  os.notifyObservers(cancelQuit, "quit-application-cancelled", null);
>       return false;
>     }
>   }
>diff -r 895e12563245 build/pgo/index.html
>--- a/build/pgo/index.html	Thu May 17 14:01:53 2012 -0400
>+++ b/build/pgo/index.html	Fri May 18 19:07:23 2012 -0700
>@@ -67,6 +67,7 @@
>      // Something aborted the quit process. 
>      if (cancelQuit.data)
>      {
>+	   os.notifyObservers(cancelQuit, "quit-application-cancelled", null);
>        return false;
>      }
>    }
>diff -r 895e12563245 mobile/android/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js
>--- a/mobile/android/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/android/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js	Fri May 18 19:07:23 2012 -0700
>@@ -129,6 +129,8 @@
>           let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>           appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>         }
>+		else
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       };
> 
>       let strings = Strings.browser;
>diff -r 895e12563245 mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/android/chrome/content/browser.js	Fri May 18 19:07:23 2012 -0700
>@@ -3535,6 +3535,8 @@
>           let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>           appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>         }
>+		else
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       }
>     }];
> 
>diff -r 895e12563245 mobile/android/components/BlocklistPrompt.js
>--- a/mobile/android/components/BlocklistPrompt.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/android/components/BlocklistPrompt.js	Fri May 18 19:07:23 2012 -0700
>@@ -66,6 +66,8 @@
>           let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>           appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>         }
>+		else
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       };
>       
>       let buttons = [{accessKey: null,
>diff -r 895e12563245 mobile/android/components/UpdatePrompt.js
>--- a/mobile/android/components/UpdatePrompt.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/android/components/UpdatePrompt.js	Fri May 18 19:07:23 2012 -0700
>@@ -149,6 +149,8 @@
>           }
>         });
>       }
>+	  else
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>     }
>   },
> 
>diff -r 895e12563245 mobile/xul/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js
>--- a/mobile/xul/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js	Fri May 18 19:07:23 2012 -0700
>@@ -129,6 +129,8 @@
>           let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>           appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>         }
>+		else
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       };
> 
>       let strings = Strings.browser;
>diff -r 895e12563245 mobile/xul/chrome/content/extensions.js
>--- a/mobile/xul/chrome/content/extensions.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/chrome/content/extensions.js	Fri May 18 19:07:23 2012 -0700
>@@ -110,6 +110,8 @@
>         let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>         appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>       }
>+	  else
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>     }
>   },
> 
>diff -r 895e12563245 mobile/xul/chrome/content/localePicker.js
>--- a/mobile/xul/chrome/content/localePicker.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/chrome/content/localePicker.js	Fri May 18 19:07:23 2012 -0700
>@@ -243,6 +243,8 @@
>         appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eForceQuit);
>         return;
>       }
>+	  else
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>     }
> 
>     // just open the window
>diff -r 895e12563245 mobile/xul/chrome/content/preferences.js
>--- a/mobile/xul/chrome/content/preferences.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/chrome/content/preferences.js	Fri May 18 19:07:23 2012 -0700
>@@ -51,6 +51,8 @@
>         let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>         appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>       }
>+	  else
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>     }
>   },
> 
>diff -r 895e12563245 mobile/xul/components/BlocklistPrompt.js
>--- a/mobile/xul/components/BlocklistPrompt.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/components/BlocklistPrompt.js	Fri May 18 19:07:23 2012 -0700
>@@ -66,6 +66,8 @@
>           let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>           appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>         }
>+		else
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       };
>       
>       let buttons = [{accessKey: null,
>diff -r 895e12563245 mobile/xul/components/UpdatePrompt.js
>--- a/mobile/xul/components/UpdatePrompt.js	Thu May 17 14:01:53 2012 -0400
>+++ b/mobile/xul/components/UpdatePrompt.js	Fri May 18 19:07:23 2012 -0700
>@@ -141,6 +141,8 @@
>         let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>         appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>       }
>+	  else
>+	    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>     }
>   },
> 
>diff -r 895e12563245 services/sync/tps/extensions/tps/modules/quit.js
>--- a/services/sync/tps/extensions/tps/modules/quit.js	Thu May 17 14:01:53 2012 -0400
>+++ b/services/sync/tps/extensions/tps/modules/quit.js	Fri May 18 19:07:23 2012 -0700
>@@ -54,6 +54,7 @@
>     // Something aborted the quit process.
>     if (cancelQuit.data)
>     {
>+	  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       return false;
>     }
>   }
>diff -r 895e12563245 testing/peptest/peptest/extension/chrome/content/quit.js
>--- a/testing/peptest/peptest/extension/chrome/content/quit.js	Thu May 17 14:01:53 2012 -0400
>+++ b/testing/peptest/peptest/extension/chrome/content/quit.js	Fri May 18 19:07:23 2012 -0700
>@@ -21,6 +21,7 @@
> 
>     // Something aborted the quit process.
>     if (cancelQuit.data) {
>+	  os.notifyObservers(cancelQuit, "quit-application-cancelled", "cancelled");
>       return false;
>     }
>   }
>diff -r 895e12563245 toolkit/components/exthelper/extApplication.js
>--- a/toolkit/components/exthelper/extApplication.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/components/exthelper/extApplication.js	Fri May 18 19:07:23 2012 -0700
>@@ -656,7 +656,10 @@
>     let quitType = aFlags & Components.interfaces.nsIAppStartup.eRestart ? "restart" : null;
>     this._obs.notifyObservers(cancelQuit, "quit-application-requested", quitType);
>     if (cancelQuit.data)
>+	{
>+	  this._obs.notifyObservers(cancelQuit, "quit-application-cancelled", quitType);
>       return false; // somebody canceled our quit request
>+	}
> 
>     let appStartup = Components.classes['@mozilla.org/toolkit/app-startup;1']
>                                .getService(Components.interfaces.nsIAppStartup);
>diff -r 895e12563245 toolkit/content/globalOverlay.js
>--- a/toolkit/content/globalOverlay.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/content/globalOverlay.js	Fri May 18 19:07:23 2012 -0700
>@@ -51,7 +51,10 @@
>     
>     // Something aborted the quit process. 
>     if (cancelQuit.data)
>+	{
>+	  os.notifyObservers(cancelQuit, "quit-application-cancelled", aData || null);
>       return false;
>+	}
>   }
>   catch (ex) { }
>   return true;
>diff -r 895e12563245 toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/mozapps/extensions/content/extensions.js	Fri May 18 19:07:23 2012 -0700
>@@ -694,7 +694,11 @@
>         Services.obs.notifyObservers(cancelQuit, "quit-application-requested",
>                                      "restart");
>         if (cancelQuit.data)
>+		{
>+		  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled", 
>+		                               "cancelled");
>           return; // somebody canceled our quit request
>+		}
> 
>         let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].
>                          getService(Ci.nsIAppStartup);
>diff -r 895e12563245 toolkit/mozapps/extensions/content/newaddon.js
>--- a/toolkit/mozapps/extensions/content/newaddon.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/mozapps/extensions/content/newaddon.js	Fri May 18 19:07:23 2012 -0700
>@@ -144,7 +144,11 @@
>   Services.obs.notifyObservers(cancelQuit, "quit-application-requested",
>                                "restart");
>   if (cancelQuit.data)
>+  {
>+    Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled",
>+	                             "cancelled");
>     return; // somebody canceled our quit request
>+  }
> 
>   window.close();
> 
>diff -r 895e12563245 toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js	Fri May 18 19:07:23 2012 -0700
>@@ -213,7 +213,10 @@
> 
>   // Something aborted the quit process.
>   if (cancelQuit.data)
>+  {
>+    os.notifyObservers(cancelQuit, "quit-application-cancelled", null);
>     return;
>+  }
> 
>   var as = Cc["@mozilla.org/toolkit/app-startup;1"].
>            getService(Ci.nsIAppStartup);
>diff -r 895e12563245 toolkit/mozapps/plugins/content/pluginInstallerWizard.js
>--- a/toolkit/mozapps/plugins/content/pluginInstallerWizard.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/mozapps/plugins/content/pluginInstallerWizard.js	Fri May 18 19:07:23 2012 -0700
>@@ -660,7 +660,7 @@
>                                .createInstance(Components.interfaces.nsISupportsPRBool);
>     os.notifyObservers(cancelQuit, "quit-application-requested", "restart");
> 
>-    // Something aborted the quit process.
>+    // If nothing aborted the quit process.
>     if (!cancelQuit.data) {
>       var nsIAppStartup = Components.interfaces.nsIAppStartup;
>       var appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"]
>@@ -668,6 +668,8 @@
>       appStartup.quit(nsIAppStartup.eAttemptQuit | nsIAppStartup.eRestart);
>       return true;
>     }
>+	else
>+	  os.notifyObservers(cancelQuit, "quit-application-cancelled", cancel);
>   }
> 
>   // don't refresh if no plugins were found or installed
>diff -r 895e12563245 toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/mozapps/update/content/updates.js	Fri May 18 19:07:23 2012 -0700
>@@ -1762,7 +1762,11 @@
> 
>     // Something aborted the quit process.
>     if (cancelQuit.data)
>+	{
>+	  Services.obs.notifyObservers(cancelQuit, "quit-application-cancelled",
>+	                               "cancelled");
>       return;
>+	}
> 
>     // If already in safe mode restart in safe mode (bug 327119)
>     if (Services.appinfo.inSafeMode) {
>diff -r 895e12563245 toolkit/xre/MacApplicationDelegate.mm
>--- a/toolkit/xre/MacApplicationDelegate.mm	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/xre/MacApplicationDelegate.mm	Fri May 18 19:07:23 2012 -0700
>@@ -351,8 +351,12 @@
> 
>   bool abortQuit;
>   cancelQuit->GetData(&abortQuit);
>+  
>   if (abortQuit)
>+  {
>+    obsServ->NotifyObservers(cancelQuit, "quit-application-cancelled", nsnull);
>     return NSTerminateCancel;
>+  }
> 
>   nsCOMPtr<nsIAppStartup> appService =
>            do_GetService("@mozilla.org/toolkit/app-startup;1");
>diff -r 895e12563245 toolkit/xre/nsNativeAppSupportUnix.cpp
>--- a/toolkit/xre/nsNativeAppSupportUnix.cpp	Thu May 17 14:01:53 2012 -0400
>+++ b/toolkit/xre/nsNativeAppSupportUnix.cpp	Fri May 18 19:07:23 2012 -0700
>@@ -155,6 +155,9 @@
> 
>     bool abortQuit;
>     cancelQuit->GetData(&abortQuit);
>+	
>+	if(abortQuit)
>+	  obsServ->NotifyObservers(cancelQuit, "quit-application-cancelled", nsnull);
>   }
> 
>   return TRUE;
>diff -r 895e12563245 tools/performance/startup/quit.js
>--- a/tools/performance/startup/quit.js	Thu May 17 14:01:53 2012 -0400
>+++ b/tools/performance/startup/quit.js	Fri May 18 19:07:23 2012 -0700
>@@ -72,6 +72,7 @@
>     // Something aborted the quit process. 
>     if (cancelQuit.data)
>     {
>+	  os.notifyObservers(cancelQuit, "quit-application-cancelled", null);
>       return false;
>     }
>   }
>diff -r 895e12563245 widget/windows/nsWindow.cpp
>--- a/widget/windows/nsWindow.cpp	Thu May 17 14:01:53 2012 -0400
>+++ b/widget/windows/nsWindow.cpp	Fri May 18 19:07:23 2012 -0700
>@@ -4485,7 +4485,10 @@
> 
>         bool abortQuit;
>         cancelQuit->GetData(&abortQuit);
>-        sCanQuit = abortQuit ? TRI_FALSE : TRI_TRUE;
>+		sCanQuit = abortQuit ? TRI_FALSE : TRI_TRUE;
>+		
>+		if(abortQuit)
>+		  obsServ->NotifyObservers(cancelQuit, "quit-application-cancelled", nsnull);
>       }
>       *aRetValue = sCanQuit ? TRUE : FALSE;
>       result = true;
My first submission of this patch did not include the unit tests that Ehsan originally included in his patch so I tried to add them by editing the submission and made the giant comment. Sorry about that, but this patch includes all the changes I made and all the changes made by Ehsan originally.
Attachment #625335 - Attachment is obsolete: true
Attachment #625335 - Flags: review?(josh)
Attachment #625342 - Flags: review?(josh)
Attachment #625342 - Attachment is obsolete: true
Attachment #625342 - Flags: review?(josh)
Attachment #625345 - Flags: review?(josh)
Comment on attachment 625345 [details] [diff] [review]
A modification to Ehsan's patch to stop the redundant useless warning

Review of attachment 625345 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks pretty good to me, but there are a couple style issues we need to fix:

* please only use spaces to indent. You're currently using a mixture of tabs and spaces; if you look at the patch here on bugzilla it should be clear where this ends up messing up the indentation.
* for places where you're adding braces ({}), they should go on the same line as the if or the else (ie. if {, else {).
* if you're adding an else to an if statement that contains braces (ie. if {...}), please add braces to the else as well, and add the else on the same line as the closing if brace (ie. } else {)
* Our style guide specifies |if (|, so please make sure that new if statements you add have a space before the opening parenthesis.

On the non-stylistic side, there's only one change needed. We should pass null as the first and third parameters of the new notifyObservers call; the current values are both inconsistent and unneeded, as they don't give any useful data to the quit-application-cancelled observers.
Attachment #625345 - Flags: review?(josh)
Attachment #625345 - Attachment is obsolete: true
Attachment #625443 - Flags: review?(josh)
Comment on attachment 625443 [details] [diff] [review]
The modified patch with the suggested changes

Review of attachment 625443 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! There are a couple more whitespace nits that will need to be fixed; you can make those changes locally and wait to upload a new version until after we get a review from Gavin.

::: browser/base/content/aboutDialog.js
@@ +267,2 @@
>          return;
> +	  }

nit: use spaces instead of tabs.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +491,4 @@
>          break;
>        case "quit-application-requested":
>          this.onQuitApplicationRequested();
> +        break;  

nit: remove trailing whitespace after the ;

::: mobile/android/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js
@@ +129,5 @@
>            let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>            appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
> +        } else {
> +          Services.obs.notifyObservers(null, "quit-application-cancelled", null);
> +		}

nit: there are tabs instead of spaces here.

::: mobile/android/components/BlocklistPrompt.js
@@ +66,5 @@
>            let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>            appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
> +        } else {
> +          Services.obs.notifyObservers(null, "quit-application-cancelled", null);
> +		}

nit: spaces instead of tabs.

::: mobile/xul/app/profile/extensions/feedback@mobile.mozilla.org/content/overlay.js
@@ +128,5 @@
>          if (cancelQuit.data == false) {
>            let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>            appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
> +        } else {
> +		  Services.obs.notifyObservers(null, "quit-application-cancelled", null);

nit: tabs.

::: toolkit/content/globalOverlay.js
@@ +55,2 @@
>        return false;
> +	}

nit: tabs.

::: toolkit/xre/nsNativeAppSupportUnix.cpp
@@ +155,5 @@
>  
>      bool abortQuit;
>      cancelQuit->GetData(&abortQuit);
> +	
> +	if (abortQuit)

nit: tabs.
Attachment #625443 - Flags: review?(josh) → review?(gavin.sharp)
Gavin? This review really fell through the cracks.
Comment on attachment 625443 [details] [diff] [review]
The modified patch with the suggested changes

Hopefully Drew can help out here. The approach seems reasonable but I haven't been able to look into specifics.

(The various places where we fire quit-application-requested and have that "Somebody canceled our quit request" code is screaming for some code sharing via JS module, but perhaps that's best addressed in a followup.)
Attachment #625443 - Flags: review?(gavin.sharp) → review?(adw)
Comment on attachment 625443 [details] [diff] [review]
The modified patch with the suggested changes

Review of attachment 625443 [details] [diff] [review]:
-----------------------------------------------------------------

Whoa, adding this new cancellation broadcast everywhere is pretty hard on maintainability.  I didn't know that requests are already broadcast at every site making the request.  Ideally there'd be some centralized service to which callers submit requests and cancellations and which makes the appropriate broadcasts.

Maybe a reasonable thing to do for now is have a function that you pass the cancel nsISupportsPRBool to: it broadcasts quit-application-canceled if the bool is true, and then it returns the bool already QI'ed so the caller can easily use it.  So instead of both checking for cancellation and broadcasting canceled everywhere, you'd only have to do the check like everybody's already doing.

There are a couple of broadcast sites in native code, so this function would have to live in an XPCOM service.  Maybe it could be part of nsIAppStartup, or the start of the aforementioned centralized quit service.

Somebody let me know if I'm being unreasonable or dumb.
Attachment #625443 - Flags: review?(adw) → review-
(In reply to Drew Willcoxon :adw from comment #26)
> Comment on attachment 625443 [details] [diff] [review]
> The modified patch with the suggested changes
> 
> Review of attachment 625443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Whoa, adding this new cancellation broadcast everywhere is pretty hard on
> maintainability.  I didn't know that requests are already broadcast at every
> site making the request.  Ideally there'd be some centralized service to
> which callers submit requests and cancellations and which makes the
> appropriate broadcasts.

Perhaps a good way to make this saner is to modify nsAppStartup::Quit to broadcast quit-application-requested and abort if someone says no, after notifying quit-application-requested, and then removing the callsites which displatch requested directly.

In that case we have to modify the following two places and get them to use nsIAppStartup:

http://mxr.mozilla.org/mozilla-central/source/build/pgo/index.html?force=1#42
http://mxr.mozilla.org/mozilla-central/source/build/bloatcycle.html?force=1#47

CCing bsmedberg so that he says I'm crazy if that's the case.

> Maybe a reasonable thing to do for now is have a function that you pass the
> cancel nsISupportsPRBool to: it broadcasts quit-application-canceled if the
> bool is true, and then it returns the bool already QI'ed so the caller can
> easily use it.  So instead of both checking for cancellation and
> broadcasting canceled everywhere, you'd only have to do the check like
> everybody's already doing.

Modifying nsAppStartup::Quit is even better, I think, since that will make it impossible for callers to forget calling that function.
This has been fixed with the advent of per-window private browsing.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: