If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 3.7a1

Status

()

Firefox
Private Browsing
P2
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: whimboo, Assigned: Ehsan)

Tracking

(Depends on: 2 bugs, 5 keywords)

3.5 Branch
Firefox 3.7a1
crash, dataloss, hang, perf, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta4-fixed, status1.9.1 wontfix)

Details

(Whiteboard: [sg:dos][mozmill], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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?
(Reporter)

Updated

8 years ago
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
(Reporter)

Comment 1

8 years ago
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]
(Reporter)

Comment 2

8 years ago
Created attachment 409908 [details]
Windbg stack
(Reporter)

Comment 3

8 years ago
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.
(Reporter)

Updated

8 years ago
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...
Created attachment 410599 [details] [diff] [review]
Patch (v1)

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
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.
Attachment #410599 - Attachment is obsolete: true
Attachment #410907 - Flags: review?(mconnor)
Attachment #410599 - Flags: review?(mconnor)
(Reporter)

Updated

8 years ago
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)
Attachment #410907 - Flags: review?(vladimir) → review+
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...
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
Last Resolved: 8 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>
status1.9.2: --- → final-fixed
Depends on: 529396
(Reporter)

Updated

8 years ago
Depends on: 529667
(Reporter)

Updated

8 years ago
Depends on: 529669

Updated

8 years ago
Depends on: 529875
(Reporter)

Updated

8 years ago
Whiteboard: [sg:dos] → [sg:dos][mozmill]
(Reporter)

Comment 17

8 years ago
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
status1.9.1: ? → wontfix
Crash Signature: [@ nsXULDocument::ResumeWalk | nsXULDocument::OnStreamComplete]
You need to log in before you can comment on or make changes to this bug.