Closed Bug 526194 Opened 15 years ago Closed 15 years ago

Switching in/out of Private Browsing mode too fast will restore windows tons of times and freeze/crash the browser [@ nsXULDocument::ResumeWalk | nsXULDocument::OnStreamComplete]

Categories

(Firefox :: Private Browsing, defect, P2)

3.5 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta4-fixed
status1.9.1 --- wontfix

People

(Reporter: whimboo, Assigned: ehsan.akhgari)

References

(Depends on 2 open bugs)

Details

(5 keywords, Whiteboard: [sg:dos][mozmill])

Crash Data

Attachments

(4 files, 1 obsolete file)

Found by running the Mozmill tests with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.5) 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)
Flags: blocking-firefox3.6?
Summary: Switching in/out of Private Browsing mode too fast will restore windows multiple times → Switching in/out of Private Browsing mode too fast will restore windows multiple times and freeze/crash the browser
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[0]: 00000000
   Parameter[1]: 00690072
Attempt to read from address 00690072
Group: core-security
Summary: Switching in/out of Private Browsing mode too fast will restore windows multiple times and freeze/crash the browser → Switching in/out of Private Browsing mode too fast will restore windows tons of times and freeze/crash the browser [@ nsXULDocument::ResumeWalk | nsXULDocument::OnStreamComplete]
Attached file Windbg stack
Attached file sessionstore.js
This is the sessionstore.js which gets created when following the steps in comment 0. It has about 2000 windows.
Keywords: testcase
Ehsan: Can you take a look at this?
Assignee: nobody → ehsan
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
Keywords: dataloss
Depends on: 526613
My work on bug 526613 is done now, and I'll have the patch ready tomorrow morning...
Attached patch Patch (v1) (obsolete) — Splinter Review
Patch + unit tests.  This needs to be applied over the patch in bug 526613.
Attachment #410599 - Flags: review?(mconnor)
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Priority: -- → P2
Attached patch Patch (v2)Splinter Review
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.
Attachment #410599 - Attachment is obsolete: true
Attachment #410907 - Flags: review?(mconnor)
Attachment #410599 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Whiteboard: [needs review mconnor]
Whiteboard: [needs review mconnor] → [needs review mconnor][sg:dos]
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.
Depends on: 528440, 528451
mconnor: ping?
Attachment #410907 - Flags: review?(mconnor) → review?(vladimir)
Attached patch Patch (v2.1)Splinter Review
Here is another version of the patch which uses the disabled property.  Which version do you prefer?  Gavin suggested it on IRC...
Attachment #412898 - Flags: review?(vladimir)
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.
Attachment #412898 - Flags: review?(vladimir)
http://hg.mozilla.org/mozilla-central/rev/9ec2968743dc
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs review mconnor][sg:dos] → [sg:dos]
Target Milestone: --- → Firefox 3.7a1
Flags: in-testsuite+
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>
Depends on: 529396
Depends on: 529667
Depends on: 529669
Depends on: 529875
Whiteboard: [sg:dos] → [sg:dos][mozmill]
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.
Group: core-security
Crash Signature: [@ nsXULDocument::ResumeWalk | nsXULDocument::OnStreamComplete]
You need to log in before you can comment on or make changes to this bug.