Found by running the Mozmill tests with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:126.96.36.199) Gecko/20091102 Firefox/3.5.5 ID:20091102134505 Users who turn off the warning when switching into Private Browsing mode can enter/leave the PB mode really quick by holding down the Cmd/Ctrl+Shift+P keys. If another windows have been opened before those will be restored multiple times. Holding down the keys for about 3-4s will hang/freeze Firefox for a long time with a high cpu load and completely fill the memory. After a couple of minutes waiting I get the warning that the virtual memory minimum limit is too low. The only way to close the browser is to kill the process via the task manager. After a couple of tries the browser crashed now. There are failures with sending the report and those are not listed under about:crashes afterward. I will try to get it into the debugger instead. Starting Firefox again with the same profile will result in the same state because session restore wants to restore all of the >100 windows again. Steps: 1. Start the browser and start the Private Browsing mode 2. Uncheck the checkbox so the PB mode will be started immediately 3. Leave the Private Browsing mode 4. Hold down Cmd/Crtl+Shift+P for about 3-4s (or longer to get the crash)
Results from the exception analysis with windbg below. Complete stack will be attached shortly. Marking as security sensitive. FAULTING_IP: xul!ns_if_addref<nsIContent *>+4 [e:\builds\moz2_slave\win32_build\build\obj-firefox\dist\include\xpcom\nsisupportsutils.h @ 114] 102868d0 8b08 mov ecx,dword ptr [eax] EXCEPTION_RECORD: ffffffff -- (.exr 0xffffffffffffffff) ExceptionAddress: 102868d0 (xul!ns_if_addref<nsIContent *>+0x00000004) ExceptionCode: c0000005 (Access violation) ExceptionFlags: 00000000 NumberParameters: 2 Parameter: 00000000 Parameter: 00690072 Attempt to read from address 00690072
Created attachment 409909 [details] sessionstore.js This is the sessionstore.js which gets created when following the steps in comment 0. It has about 2000 windows.
Ehsan: Can you take a look at this?
Oh, I was already looking at this. This seems like a serious issue which I think should block. I have a half-baked patch which I think would be ready by the end of today. The problem is that the user is allowed to change the private browsing status before another transition is fully finished. I have a fix which disables the private browsing command until the new session is fully restored. We were already handling a weaker type of this protection (not allowing to recursively set the private browsing status), but this fix should make any such future problems go away as well. This needs a sessionstore fix for which I'm going to file a bug in a moment.
Of course, the fix that I'm talking about is only restricted to the private browsing code. Basically we make it impossible for this issue to surface through private browsing. The security impact of this is a denial of service, in which an external app can sent successive Ctrl+Shift+P sequences to the Firefox process which would effectively result in: a) user's browser crashing b) other apps running on the desktop face similar problems (crash or DOS) because of exhaustion of physical memory c) dataloss in the sense that the user's session would become impossible to restore unless they have the expertise to manually edit their sessionstore.js
My work on bug 526613 is done now, and I'll have the patch ready tomorrow morning...
Created attachment 410599 [details] [diff] [review] Patch (v1) Patch + unit tests. This needs to be applied over the patch in bug 526613.
Created attachment 410907 [details] [diff] [review] Patch (v2) I pushed the patch to the try server which revealed some test failings. Most of them were test problems, and one was actually a minor bug in my patch, which I fixed them all. Here's the new patch which should pass the entire test suite.
Bug 528440 and bug 528451 together can affect the fix for this bug in extreme cases. I'm going to request those bugs to block the release of Firefox 3.6 as well.
Created attachment 412898 [details] [diff] [review] Patch (v2.1) Here is another version of the patch which uses the disabled property. Which version do you prefer? Gavin suggested it on IRC...
Comment on attachment 412898 [details] [diff] [review] Patch (v2.1) Actually I'm going to go with the version 2 of the patch, since it's already reviewed, and other places in browser.js use the same convention.
I landed a test bustage fix as <http://hg.mozilla.org/mozilla-central/rev/60ad411785a3>.
Landed both patches combined on 1.9.2: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/da8f87127f2d>
I would like to verify this fix but because of bug 529669 I'm not able to. I will have to wait until bug 529669 is fixed.