Closed Bug 507114 Opened 14 years ago Closed 14 years ago

[Win] Topcrasher for Firefox 3.5.1 [@ memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)][@ nsObserverList::FillObserverArray]

Categories

(Core :: XPCOM, defect, P1)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta4-fixed
blocking1.9.1 --- .7+
status1.9.1 --- .7-fixed

People

(Reporter: whimboo, Assigned: sicking)

Details

(Keywords: crash, fixed1.9.0.18, topcrash, Whiteboard: [crashkill-fix][verify on both 1.9.1.7 and 1.9.1.8])

Crash Data

Attachments

(4 files, 1 obsolete file)

Possible fix
1.37 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Fix part 2
3.01 KB, patch
jst
: review+
Details | Diff | Splinter Review
observerService changes, 191 branch
1.37 KB, patch
dveditz
: approval1.9.1.7+
dveditz
: approval1.9.1.8+
dveditz
: approval1.9.0.18+
Details | Diff | Splinter Review
js low-on-memory changes, 1.9.1 branch
3.02 KB, patch
dveditz
: approval1.9.1.7+
dveditz
: approval1.9.1.8+
Details | Diff | Splinter Review 
There is a #1 topcrasher on 1.9.1 with the following stack. This bug has been filed by request on bug 494617.

Some comments:
* Playing games at Iwon.com - lost all my coins!
* Took me off the scrabble game i was playing through the facebook link. Was the first game that i was playing for the night, hadnt even put my first word up.
* im not understanding why i keep getting these low memory warnings i have deleted firfox and reinstalled it and still get this warnings. why dose this happen on this site.

Stack:
0  	mozcrt19.dll  	memmove  	MEMCPY.ASM:188
1 	xul.dll 	nsTArray_base::ShiftData 	obj-firefox/xpcom/build/nsTArray.cpp:173
2 	xul.dll 	nsObserverList::FillObserverArray 	xpcom/ds/nsObserverList.cpp:110
3 	xul.dll 	xul.dll@0x8aea63
Do you have a testcase that reproduces the crash?
Sadly not. A list of URLs would be helpful we could use to simulate a low memory situtation which could raise this crash.
blocking1.9.1: ? → needed
status1.9.1: --- → ?
I'd prefer this be a security bug until someone says it's not.
Group: core-security
I suspect URLs aren't going to help much on this bug.  There seems to be a wide distribution of time to crash with the average crash taking longer than 2 hours, a wide variety of versions, and the sites where the crash is seen is pretty much reflective of general web surfing.

347246 (hours) total uptime  for these crashes 134.571 (minutes) avg time since last crash 

distribution of versions where the crash was found on 20090813-crashdata.csv
 201 Firefox 3.5.2
  42 Firefox 3.5.1
  40 Firefox 3.5
  34 Firefox 3.0.13
   4 Firefox 3.1b3
   4 Firefox 3.0.11
   2 Firefox 3.5b4
   2 Firefox 3.0.1
   1 Firefox 3.5b99
   1 Firefox 3.1b2
   1 Firefox 3.0.8
   1 Firefox 3.0.12
   1 Firefox 3.0

kinds of sites found with this signature:
       6

types of sites
 278 http:
  23 
  20 \N
  10 https:
   2 about:blank
   1 about:sessionrestore

domains of sites
  30 http://www.facebook.com
  25 http://apps.facebook.com
  23 //
  20 \N//
  16 http://www.orkut.com.br
  11 http://www.youtube.com
   4 http://www.schuelervz.net
   4 http://www.myspace.com
   4 http://mail.live.com
   4 http://mail.google.com
   3 http://login.live.com
   2 https://www.google.com
   2 https://mail.google.com
   2 http://www.yandex.ru
   2 http://www.tagged.com
   2 http://www.picnik.com
   2 http://www.orkut.co.in
   2 http://www.google.com.br
   2 http://www.google.com
   2 http://www.friendster.com
   2 http://www.fotka.pl
   2 http://webmail.aol.com
   2 http://get.adobe.com
   2 about:blank//
  many more sites with a single report...
one test case might be to  open up lots of tabs and windows and cycle around on these web apps and pages for a few hours 

  8 http://apps.facebook.com/farmtown
  4 http://www.orkut.com.br/Main#Home.aspx
  4 http://www.facebook.com/home.php?#
  4 http://apps.facebook.com/restaurantcity
  4 http://apps.facebook.com/onthefarm
  3 http://www.facebook.com/search
  3 http://www.facebook.com/home.php?ref=home
  3 http://www.facebook.com/home.php#
  3 http://mail.live.com/
  3 http://mail.google.com/mail
  2 https://www.google.com/accounts
  2 https://mail.google.com/mail
  2 http://www.yandex.ru/
  2 http://www.facebook.com/home.php?ref=home#
  2 http://www.facebook.com/home.php?
  2 http://webmail.aol.com/44054
  2 http://apps.facebook.com/petsociety
  2 about:blank///
  1 https://www3.netbank.commbank.com.au/netbank
  1 https://www.r-bank.pl/newsbi
  1 https://www.paypal.com/us
  1 https://www.labanquepostale.fr/index.html
  1 https://myanswers.radioshack.com/psp
  1 https://my.yorku.ca/user
  1 http://yobanbe.zing.vn/yobanbe
  1 http://www3.searchlabel.com
  1 http://www12.24h.com.vn/
  1 http://www.youtube.com/watch?v=lrRWfLFHzow&NR=1
  1 http://www.youtube.com/watch?v=iiu8Hfkzi_s&NR=1
  1 http://www.youtube.com/watch?v=d3YJrpDQi_I
  1 http://www.youtube.com/watch?v=S2O5xyykbJs
  1 http://www.youtube.com/watch?v=RVg1bqlu1qU
  1 http://www.youtube.com/watch?v=IACc6PhHicc
  1 http://www.youtube.com/watch?v=E4aQpczZC00
  1 http://www.youtube.com/watch?v=6neV2Nwoo1o&feature=related
  1 http://www.youtube.com/watch?v=0MoS08DF07Q
  1 http://www.youtube.com/
I don't think this should be security-sensitive.  There's no information here that would help an attacker.
And meanwhile, users have no way to know that we need help with this (e.g. catching it in a debugger).
bug 485799 (open) and bug 494617 (closed) have similar signatures if not stacks.
(In reply to comment #8)
> bug 485799 (open) and bug 494617 (closed) have similar signatures if not
> stacks.

See bug 494617 comment 17 why that bug has been filed.
Jesse says not security-sensitive, so back we go.
Group: core-security
Is this a dupe of bug 519771? Since that one is crash #33 and the signature in this summary [@ memmove | nsTArray_base::ShiftData | nsObserverList::FillObserverArray] doesn't show up in the crash list, and there's no breakpad links here to prove or disprove it I'm going to assume so.

Duping forward because Jonas is actively working on the new one.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: blocking1.9.2?
Resolution: --- → DUPLICATE
Reopening as I think bug 519771 actually is several different crashers, and this is a specific one.
Assignee: nobody → jonas
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
It seems like this is happening while trying to notify "domwindowopened" observers while opening an alert dialog.
Attached patch Possible fix — — Splinter Review 
So this might fix things.

What's happening at the crash is that on the line:

mObservers.RemoveElementAt(i);

i is 0, but the array is empty. And since nsTArray isn't out-of-bounds safe this crashes.

Also noteworthy is that we're always in the process of sending out a "domwindowadded" notification while bringing up an alert dialog. And the reason we're bringing up the dialog is because we're almost out of memory.

The theory is that this happens due to the call to do_QueryReferent actually ends up executing code which removes observers. Jst had a good theory that the observer behind the weakref is a javascript one. That might be behaving weird since we're in a low-memory situation, and that's probably what's causing the list of observers to change during the do_QueryReferent call.


In any case, this patch should make us more resistant in the face of unexpected do_QueryReferent implementations.
Attachment #412062 - Flags: review?
Oh, I should mention that the patch makes us do memory copying during notifications. I pushed a tryserver build but didn't see any noticable slowdown.

Things could be made more efficient, however that would be a riskier patch. So I'd like to land this on trunk and branch for now, and then maybe write a quicker patch for trunk later.
Attachment #412062 - Flags: review? → review?(benjamin)
Attached patch Fix part 2 — — Splinter Review 
Additionally, I think we should take this patch.

Currently when we detect a low memory condition we throw up a warning *dialog* and then throw a uncatchable exception. This seems extremely risky to me. Opening a dialog requires a lot of system resources and executes a lot of code, so doing that when we know we're low on memory seems very risky.

This patch makes us instead just send a message to the nsIConsoleService. Even this seems a bit scary, but we seemed to get fairly far when putting up a alert-dialog, so I think it should be fine.

Alternatively, we could simply flip the "dom.prevent_oom_dialog" pref.
Attachment #412092 - Flags: review?(jst)
Whiteboard: [crashkill][needs review bsmedberg][needs review jst]
for reference w/ the js console open, when we used to throw oom messages at it, bad things happened (sad loops). that said, i agree that tossing up a dialog is a bad idea under low memory conditions.
So two things make it different here:

* We're not *out* of memory. This code doesn't run on a failed memory allocation, rather when we're running low on memory.
* This code doesn't run for chrome scripts. So no risk of recursive errors due to nsIConsoleService running low on memory.
Attachment #412092 - Flags: review?(jst) → review+
Comment on attachment 412092 [details] [diff] [review]
Fix part 2

Yup, I agree that we should take this change, and so does dougt who I believe wrote this in the first place.
Attachment #412062 - Flags: review?(benjamin) → review+
Would really like a test for this...
Flags: in-testsuite?
Fixed on trunk and 1.9.2:

http://hg.mozilla.org/mozilla-central/rev/51396f6c9f20
http://hg.mozilla.org/mozilla-central/rev/c3d143564df6

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cb98ced43779
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/00b8d55251ad
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED
Whiteboard: [crashkill][needs review bsmedberg][needs review jst] → [crashkill]
Nominating for 1.9.1.x. (It was needed, but we should probably block on it for .7 and this gets it back on our queries.)
blocking1.9.1: needed → ?
Whiteboard: [crashkill] → [crashkill-fix]
Attachment #412757 - Attachment is patch: true
Attachment #412757 - Attachment mime type: application/octet-stream → text/plain
Probably the wrong patch has been added on comment 24.
Thanks! This is the correct patch
Attachment #412757 - Attachment is obsolete: true
Attachment #412893 - Flags: approval1.9.1.7?
Attachment #412757 - Flags: approval1.9.1.7?
blocking1.9.1: ? → .7+
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #412756 - Flags: approval1.9.1.7? → approval1.9.1.7+
Comment on attachment 412893 [details] [diff] [review]
js low-on-memory changes, 1.9.1 branch

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #412893 - Flags: approval1.9.1.7? → approval1.9.1.7+
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

do we need both patches on 1.9.0 as we did on 1.9.1, or did you really only want to land this one?
Only want one. The second patch changes code that was added after 1.9.0
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

Approved for 1.9.0.17, a=dveditz for release-drivers
Attachment #412756 - Flags: approval1.9.0.17? → approval1.9.0.17+
Checked in to 1.9.0 branch

Checking in xpcom/ds/nsObserverList.cpp;
/cvsroot/mozilla/xpcom/ds/nsObserverList.cpp,v  <--  nsObserverList.cpp
new revision: 3.44; previous revision: 3.43
Keywords: fixed1.9.0.17
Summary: [Win] Topcrasher for Firefox 3.5.1 [@ memmove | nsTArray_base::ShiftData | nsObserverList::FillObserverArray] → [Win] Topcrasher for Firefox 3.5.1 [@ memmove][@ nsTArray_base::ShiftData][@ nsObserverList::FillObserverArray]
Target Milestone: --- → mozilla1.9.3a1
Summary: [Win] Topcrasher for Firefox 3.5.1 [@ memmove][@ nsTArray_base::ShiftData][@ nsObserverList::FillObserverArray] → [Win] Topcrasher for Firefox 3.5.1 [@ memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)][@ nsObserverList::FillObserverArray]
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

The previous .7/.17 releases were renamed .8/.18, approved also for the NEW 1.9.1.7
and 1.9.0.17, a=dveditz for release-drivers

This means checking in on the 1.9.1.6/1.9.0.16 relbranches:

hg(191): GECKO1916_20091130_RELBRANCH 
cvs(190): GECKO190_20091130_RELBRANCH
Attachment #412756 - Flags: approval1.9.1.7+
Attachment #412756 - Flags: approval1.9.0.17+
Attachment #412893 - Flags: approval1.9.1.7+
Please change status1.9.1 from .8-fixed to .7-fixed when it's landed on the relbranch. QA will verify on both before adding the verified keyword.

For the 1.9.0 tree please add the fixed1.9.0.17 keyword when you land, and leave the fixed1.9.0.18 keyword there as well.
blocking1.9.1: .8+ → .7+
Whiteboard: [crashkill-fix] → [crashkill-fix][verify on both 1.9.1.7 and 1.9.1.8]
Dan, I'm confused. This was already landed on the 1.9.1 and 1.9.0 branchs. Do you want me to backport to an additional branch?

I.e. if the 1.9.1.7 release was renamed to 1.9.1.8, why do we want to backport to the 1.9.1.7 release?
(In reply to comment #36)
> Dan, I'm confused. This was already landed on the 1.9.1 and 1.9.0 branchs. Do
> you want me to backport to an additional branch?

This needs to land on both relbranches. It was determined to be important enough to land in the short-cycle 1.9.1.7 and 1.9.0.17. (See also comment 34.)
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

Since this isn't a topcrash on the 1.9.0 branch we don't need this on the 1.9.0 relbranch after all, just the 1.9.1.6-based one.
Attachment #412756 - Flags: approval1.9.0.17+
Whiteboard: [crashkill-fix][verify on both 1.9.1.7 and 1.9.1.8] → [crashkill-fix][needs relbranch landing][verify on both 1.9.1.7 and 1.9.1.8]
Whiteboard: [crashkill-fix][needs relbranch landing][verify on both 1.9.1.7 and 1.9.1.8] → [crashkill-fix][verify on both 1.9.1.7 and 1.9.1.8]
(Pretty sure this is .7-fixed)
Yes, Blake pushed to the 1.9.1.7 branch. Thanks Blake!
Crash Signature: [@ memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)] [@ nsObserverList::FillObserverArray]
You need to log in before you can comment on or make changes to this bug.