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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
XPCOM
P1
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: whimboo, Assigned: sicking)

Tracking

({crash, fixed1.9.0.18, topcrash})

Trunk
mozilla1.9.3a1
x86
Windows XP
crash, fixed1.9.0.18, topcrash
Points:
---
Bug Flags:
blocking1.9.2 +
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 beta4-fixed, blocking1.9.1 .7+, status1.9.1 .7-fixed)

Details

(Whiteboard: [crashkill-fix][verify on both 1.9.1.7 and 1.9.1.8], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 2

8 years ago
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

Comment 4

8 years ago
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

8 years ago
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

8 years ago
I don't think this should be security-sensitive.  There's no information here that would help an attacker.

Comment 7

8 years ago
And meanwhile, users have no way to know that we need help with this (e.g. catching it in a debugger).

Comment 8

8 years ago
bug 485799 (open) and bug 494617 (closed) have similar signatures if not stacks.
(Reporter)

Comment 9

8 years ago
(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
status1.9.1: ? → wanted
Keywords: testcase-wanted
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
Last Resolved: 8 years ago
Flags: blocking1.9.2?
Resolution: --- → DUPLICATE
Duplicate of bug: 519771
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.
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.
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)
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.
Attachment #412092 - Flags: review?(jst)
Whiteboard: [crashkill][needs review bsmedberg][needs review jst]

Comment 17

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

Updated

8 years ago
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
Last Resolved: 8 years ago8 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 → ?
Created attachment 412756 [details] [diff] [review]
observerService changes, 191 branch
Attachment #412756 - Flags: approval1.9.1.7?
Whiteboard: [crashkill] → [crashkill-fix]
Created attachment 412757 [details] [diff] [review]
js low-on-memory changes, 1.9.1 branch
Attachment #412757 - Flags: approval1.9.1.7?
Attachment #412757 - Attachment is patch: true
Attachment #412757 - Attachment mime type: application/octet-stream → text/plain

Comment 25

8 years ago
Probably the wrong patch has been added on comment 24.
Created attachment 412893 [details] [diff] [review]
js low-on-memory changes, 1.9.1 branch

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+
Attachment #412756 - Flags: approval1.9.0.17?
Fixed on 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c0f80eda8bea
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c512a6179e5
status1.9.1: wanted → .7-fixed
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
(Reporter)

Updated

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

Comment 37

8 years ago
(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]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3d5856f34c3f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fda726574931
status1.9.1: .8-fixed → .7-fixed

Updated

8 years ago
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]
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.