Last Comment Bug 507114 - [Win] Topcrasher for Firefox 3.5.1 [@ memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)][@ nsObserverList::FillObserverArray]
: [Win] Topcrasher for Firefox 3.5.1 [@ memmove | nsTArray_base::ShiftData(unsi...
Status: RESOLVED FIXED
[crashkill-fix][verify on both 1.9.1....
: crash, fixed1.9.0.18, topcrash
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: P1 critical (vote)
: mozilla1.9.3a1
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-29 06:07 PDT by Henrik Skupin (:whimboo)
Modified: 2015-10-16 11:37 PDT (History)
18 users (show)
mbeltzner: blocking1.9.2+
benjamin: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta4-fixed
.7+
.7-fixed


Attachments
Possible fix (1.37 KB, patch)
2009-11-12 13:32 PST, Jonas Sicking (:sicking) PTO Until July 5th
benjamin: review+
Details | Diff | Review
Fix part 2 (3.01 KB, patch)
2009-11-12 15:16 PST, Jonas Sicking (:sicking) PTO Until July 5th
jst: review+
Details | Diff | Review
observerService changes, 191 branch (1.37 KB, patch)
2009-11-16 19:13 PST, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: approval1.9.1.7+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Review
js low-on-memory changes, 1.9.1 branch (1.37 KB, patch)
2009-11-16 19:15 PST, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
js low-on-memory changes, 1.9.1 branch (3.02 KB, patch)
2009-11-17 11:16 PST, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: approval1.9.1.7+
dveditz: approval1.9.1.8+
Details | Diff | Review

Description Henrik Skupin (:whimboo) 2009-07-29 06:07:30 PDT
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
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-07-29 07:02:23 PDT
Do you have a testcase that reproduces the crash?
Comment 2 Henrik Skupin (:whimboo) 2009-07-29 07:07:06 PDT
Sadly not. A list of URLs would be helpful we could use to simulate a low memory situtation which could raise this crash.
Comment 3 Samuel Sidler (old account; do not CC) 2009-07-29 11:46:10 PDT
I'd prefer this be a security bug until someone says it's not.
Comment 4 chris hofmann 2009-08-17 17:02:28 PDT
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...
Comment 5 chris hofmann 2009-08-17 17:38:14 PDT
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/
Comment 6 Jesse Ruderman 2009-08-19 16:11:38 PDT
I don't think this should be security-sensitive.  There's no information here that would help an attacker.
Comment 7 Jesse Ruderman 2009-08-19 16:12:08 PDT
And meanwhile, users have no way to know that we need help with this (e.g. catching it in a debugger).
Comment 8 chris hofmann 2009-08-19 17:38:02 PDT
bug 485799 (open) and bug 494617 (closed) have similar signatures if not stacks.
Comment 9 Henrik Skupin (:whimboo) 2009-08-20 00:25:29 PDT
(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.
Comment 10 Samuel Sidler (old account; do not CC) 2009-08-28 12:14:57 PDT
Jesse says not security-sensitive, so back we go.
Comment 11 Daniel Veditz [:dveditz] 2009-10-07 16:35:46 PDT
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.

*** This bug has been marked as a duplicate of bug 519771 ***
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-08 15:42:40 PDT
Reopening as I think bug 519771 actually is several different crashers, and this is a specific one.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-06 17:52:14 PST
It seems like this is happening while trying to notify "domwindowopened" observers while opening an alert dialog.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 13:32:48 PST
Created attachment 412062 [details] [diff] [review]
Possible fix

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.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 13:49:28 PST
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.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 15:16:36 PST
Created attachment 412092 [details] [diff] [review]
Fix part 2

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.
Comment 17 timeless 2009-11-12 17:29:52 PST
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.
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 17:35:52 PST
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.
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-16 13:27:39 PST
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.
Comment 20 Benjamin Smedberg [:bsmedberg] 2009-11-16 13:44:00 PST
Would really like a test for this...
Comment 22 Samuel Sidler (old account; do not CC) 2009-11-16 17:25:39 PST
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.)
Comment 23 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-16 19:13:37 PST
Created attachment 412756 [details] [diff] [review]
observerService changes, 191 branch
Comment 24 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-16 19:15:28 PST
Created attachment 412757 [details] [diff] [review]
js low-on-memory changes, 1.9.1 branch
Comment 25 Guilherme Lima 2009-11-17 08:33:07 PST
Probably the wrong patch has been added on comment 24.
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-17 11:16:26 PST
Created attachment 412893 [details] [diff] [review]
js low-on-memory changes, 1.9.1 branch

Thanks! This is the correct patch
Comment 27 Daniel Veditz [:dveditz] 2009-11-30 14:56:42 PST
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 28 Daniel Veditz [:dveditz] 2009-11-30 14:57:12 PST
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
Comment 30 Daniel Veditz [:dveditz] 2009-12-02 15:45:04 PST
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?
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2009-12-02 15:57:35 PST
Only want one. The second patch changes code that was added after 1.9.0
Comment 32 Daniel Veditz [:dveditz] 2009-12-14 14:28:22 PST
Comment on attachment 412756 [details] [diff] [review]
observerService changes, 191 branch

Approved for 1.9.0.17, a=dveditz for release-drivers
Comment 33 Jonas Sicking (:sicking) PTO Until July 5th 2009-12-14 14:47:38 PST
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
Comment 34 Daniel Veditz [:dveditz] 2009-12-18 10:41:22 PST
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
Comment 35 Daniel Veditz [:dveditz] 2009-12-18 11:04:19 PST
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.
Comment 36 Jonas Sicking (:sicking) PTO Until July 5th 2009-12-18 11:24:02 PST
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?
Comment 37 Samuel Sidler (:ss) 2009-12-18 12:17:20 PST
(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 38 Daniel Veditz [:dveditz] 2009-12-18 14:40:54 PST
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.
Comment 40 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-21 12:49:33 PST
(Pretty sure this is .7-fixed)
Comment 41 Jonas Sicking (:sicking) PTO Until July 5th 2009-12-21 13:27:08 PST
Yes, Blake pushed to the 1.9.1.7 branch. Thanks Blake!

Note You need to log in before you can comment on or make changes to this bug.