The default bug view has changed. See this FAQ.

crash in AppendUTF<n>toUTF<m> with abort message: "OOM: file e:\...\nsTSubstring.h, line 533" or in nsPrefBranch::GetComplexValue with abort message: "e:/.../modules/libpref/src/nsPrefBranch.cpp, line 330" mainly with Giant Savings

VERIFIED FIXED in Firefox 23

Status

()

Core
Preferences: Backend
--
critical
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Scoobidiver (away), Assigned: khuey)

Tracking

(Depends on: 1 bug, 4 keywords)

Trunk
mozilla23
addon-compat, crash, steps-wanted, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+, firefox20+ wontfix, firefox21 wontfix, firefox22 wontfix, firefox23 verified, relnote-firefox -)

Details

(Whiteboard: [no-nag], crash signature)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
It jumped up to #52 top browser crasher in 18.0.1 and #47 in 19.0b3.
It has the same abort message as bug 814007, bug 802152, bug 776473, bug 767343.

It's correlated to recent versions of crossrider extensions:
  mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)|EXCEPTION_BREAKPOINT (212 crashes)
     30% (63/212) vs.   0% (623/176971) crossriderapp4479@crossrider.com (Giant Savings)
          0% (0/212) vs.   0% (1/176971) 0.81.12
          0% (0/212) vs.   0% (3/176971) 0.83.20
          0% (0/212) vs.   0% (1/176971) 0.85.42
          0% (0/212) vs.   0% (1/176971) 0.85.43
         30% (63/212) vs.   0% (617/176971) 0.86.44
     13% (27/212) vs.   0% (117/176971) crossriderapp4639@crossrider.com (SavingsApp)
          0% (0/212) vs.   0% (2/176971) 0.84.35
         13% (27/212) vs.   0% (115/176971) 0.86.44
     16% (34/212) vs.   4% (7440/176971) plugin@yontoo.com (1.20.00)

Signature 	mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&) More Reports Search
UUID	69aadc97-4b29-4455-8d70-c12902130130
Date Processed	2013-01-30 13:13:52
Uptime	6953
Install Age	1.5 weeks since version was first installed.
Install Time	2013-01-19 19:09:20
Product	Firefox
Version	18.0.1
Build ID	20130116073211
Release Channel	release
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 6
Crash Reason	EXCEPTION_BREAKPOINT
Crash Address	0xfe1988
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x06e4, AdapterSubsysID: 10911019, AdapterDriverVersion: 6.14.12.6658
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers+ xpcom_runtime_abort(###!!! ABORT: OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529)
Processor Notes 	sp-processor10.phx1.mozilla.com_20265:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x06e4
Total Virtual Memory	2147352576
Available Virtual Memory	762540032
System Memory Use Percentage	99
Available Page File	2080358400
Available Physical Memory	16498688

Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:23
1 	xul.dll 	NS_DebugBreak_P 	xpcom/base/nsDebugImpl.cpp:410
2 	xul.dll 	AppendUTF8toUTF16 	xpcom/string/src/nsReadableUtils.cpp:194
3 	xul.dll 	xul.dll@0xd46ef0 	
4 	xul.dll 	XPCConvert::NativeData2JS 	js/xpconnect/src/xpcprivate.h:3348
5 	xul.dll 	nsMemory::Clone 	obj-firefox/xpcom/build/nsMemory.cpp:31
6 	xul.dll 	XPCConvert::JSData2Native 	js/xpconnect/src/XPCConvert.cpp:459
7 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2383
8 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469
9 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:1122
10 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2529
11 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:324
12 	mozjs.dll 	UncachedInlineCall 	js/src/methodjit/InvokeHelpers.cpp:363
13 	mozjs.dll 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:451
14 	mozjs.dll 	js::mjit::stubs::UncachedLoweredCall 	js/src/methodjit/InvokeHelpers.cpp:414
15 	mozjs.dll 	js::mjit::stubs::CompileFunction 	js/src/methodjit/InvokeHelpers.cpp:244
16 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:1122
17 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2529
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak_P+|+AppendUTF8toUTF16%28nsACString_internal+const%26%2C+nsAString_internal%26%29
(Reporter)

Comment 1

4 years ago
It's now #18 top browser crasher in 18.0.1, #6 in 19.0b4, #24 in 20.0a2, and #37 in 21.0a1.

It's still correlated to Giant Savings and other known malware.
* 18.0.1:
     49% (552/1133) vs.   1% (1506/204664) crossriderapp4479@crossrider.com (Giant Savings)
          0% (1/1133) vs.   0% (2/204664) 0.81.12
          0% (1/1133) vs.   0% (5/204664) 0.81.13
          0% (1/1133) vs.   0% (2/204664) 0.83.28
          0% (1/1133) vs.   0% (2/204664) 0.85.42
         41% (460/1133) vs.   1% (1260/204664) 0.86.44
          5% (61/1133) vs.   0% (169/204664) 0.87.66
          2% (27/1133) vs.   0% (57/204664) 0.88.67
     19% (216/1133) vs.   4% (8375/204664) plugin@yontoo.com (1.20.00)
     15% (167/1133) vs.   0% (408/204664) crossriderapp4639@crossrider.com (SavingsApp)
          0% (2/1133) vs.   0% (2/204664) 0.82.14
         15% (165/1133) vs.   0% (405/204664) 0.86.44
     16% (182/1133) vs.   3% (7053/204664) ffxtlbr@babylon.com
          0% (2/1133) vs.   0% (20/204664) 1.1.3
          7% (79/1133) vs.   1% (1689/204664) 1.1.9
          2% (24/1133) vs.   1% (1283/204664) 1.2.0
          7% (77/1133) vs.   2% (4044/204664) 1.5.0
     13% (147/1133) vs.   3% (5345/204664) ffxtlbr@funmoods.com
          5% (61/1133) vs.   0% (985/204664) 1.5.0
          8% (86/1133) vs.   2% (4360/204664) 1.5.1

* 19.0 Beta:
     43% (73/168) vs.   0% (210/56681) crossriderapp4479@crossrider.com (Giant Savings)
          2% (4/168) vs.   0% (4/56681) 0.83.33
         37% (62/168) vs.   0% (176/56681) 0.86.44
          4% (6/168) vs.   0% (20/56681) 0.87.66
          1% (1/168) vs.   0% (10/56681) 0.88.67
     26% (43/168) vs.   4% (2530/56681) ffxtlbr@babylon.com
          7% (12/168) vs.   1% (436/56681) 1.1.9
          2% (3/168) vs.   1% (307/56681) 1.2.0
         17% (28/168) vs.   3% (1777/56681) 1.5.0
     17% (29/168) vs.   5% (2632/56681) plugin@yontoo.com (1.20.00)
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
Keywords: topcrash

Comment 2

4 years ago
Jorge - how do you feel about blocking these malware add-ons? Have we done so in the past?
Flags: needinfo?(jorge)
(Reporter)

Comment 3

4 years ago
Giant Savings (http://giant-savings.com/) displays ad popups (it promises to offer you coupon when shopping online).
Other pieces of adware are not enough correlated to blocklist them.
(Reporter)

Comment 4

4 years ago
With combined signatures, it's even worse: #10 in 18.0.1, #4 in 19.0b4, and #9 in 20.0a2.

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak_P+|+AppendUTF16toUTF8%28nsAString_internal+const%26%2C+nsACString_internal%26%29
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&)]
Summary: crash in AppendUTF8toUTF16 with abort message: "OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529" mainly with crossrider extensions → crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529" mainly with crossrider extensions
(Reporter)

Updated

4 years ago
See Also: → bug 837497
Summary: crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529" mainly with crossrider extensions → crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529" mainly with Giant Savings
(In reply to Alex Keybl [:akeybl] from comment #2)
> Jorge - how do you feel about blocking these malware add-ons? Have we done
> so in the past?

It looks like the strongest correlation is Giant Savings. The others are very widely installed, so it's possible they aren't the real cause. Incidentally, see blocklist bug 835664 for Yontoo and bug 804847 for Funmoods.

I'll try contacting the Giant Savings and Crossrider people
Flags: needinfo?(jorge)
Crossrider pushed an update that may help with this. We should check the crash stats again in a couple of days and see if there's any change.

Updated

4 years ago
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected

Comment 7

4 years ago
The volume of this has been going down since 2013-02-05 data, but we need to keep watching it for a few days longer.

Updated

4 years ago
tracking-firefox19: ? → -
tracking-firefox20: ? → -
(Reporter)

Comment 8

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #7)
> The volume of this has been going down since 2013-02-05 data, but we need to
> keep watching it for a few days longer.
I don't know where you've got your data but it's still high with no downward tendency, currently #2 top browser crasher in 18.0.2, #4 in 19.0b4 and #3 in 20.0a2 over the last day.

(In reply to Jorge Villalobos [:jorgev] from comment #6)
> Crossrider pushed an update that may help with this. We should check the
> crash stats again in a couple of days and see if there's any change.
The latest version of those extensions is still affected:
  mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)|EXCEPTION_BREAKPOINT (1780 crashes)
     44% (792/1780) vs.   1% (1465/122260) crossriderapp4479@crossrider.com
          0% (0/1780) vs.   0% (2/122260) 0.81.12
          0% (1/1780) vs.   0% (2/122260) 0.81.13
          0% (2/1780) vs.   0% (2/122260) 0.83.28
          0% (0/1780) vs.   0% (1/122260) 0.83.33
          0% (3/1780) vs.   0% (3/122260) 0.84.36
          0% (8/1780) vs.   0% (15/122260) 0.86.44
          1% (9/1780) vs.   0% (20/122260) 0.87.66
          1% (16/1780) vs.   0% (33/122260) 0.88.67
         42% (753/1780) vs.   1% (1387/122260) 0.88.83
     18% (321/1780) vs.   1% (629/122260) crossriderapp4637@crossrider.com
          0% (3/1780) vs.   0% (4/122260) 0.83.24
          0% (1/1780) vs.   0% (1/122260) 0.83.26
          0% (1/1780) vs.   0% (1/122260) 0.85.34
         18% (316/1780) vs.   1% (617/122260) 0.87.43
          0% (0/1780) vs.   0% (6/122260) 0.88.58
     12% (210/1780) vs.   0% (392/122260) crossriderapp4493@crossrider.com
          0% (0/1780) vs.   0% (2/122260) 0.81.12
          0% (1/1780) vs.   0% (1/122260) 0.85.39
          0% (5/1780) vs.   0% (8/122260) 0.85.40
          9% (163/1780) vs.   0% (315/122260) 0.88.64
          2% (41/1780) vs.   0% (66/122260) 0.88.77
tracking-firefox19: - → ?
tracking-firefox20: - → ?

Comment 9

4 years ago
(In reply to Scoobidiver from comment #8)
> (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #7)
> > The volume of this has been going down since 2013-02-05 data, but we need to
> > keep watching it for a few days longer.
> I don't know where you've got your data

I saw this in terms of crashes per M ADI in my explosiveness reports but the trend didn't hold up, as you also mentioned.
tracking-firefox19: ? → +
tracking-firefox20: ? → +
Because we don't know yet if this requires work on our end, leaving it nominated for tracking while more info is gathered.
status-firefox19: affected → wontfix
tracking-firefox19: + → -
tracking-firefox20: + → ?
Benjamin was planning on taking a look to see if we could make any recommendations to the Giant Savings devs.

I'd actually like to keep this on the FF19 tracking list in case we feel a blocklist is necessary.
Assignee: nobody → benjamin
status-firefox19: wontfix → affected
tracking-firefox19: - → +
tracking-firefox20: ? → +
Bah, I wrote a big bug comment here but it apparently got lost. In case it's not clear, the bug here is that we're trying to create a string which is so long that we're failing to allocate memory for it and then aborting.

The stack is unfortunately kinda busted above AppendUTF8toUTF16 (it's even worse with MSVC), but I went in using dump_lookup and the most likely caller is

http://hg.mozilla.org/releases/mozilla-release/annotate/eecd28b7ba09/modules/libpref/src/nsPrefBranch.cpp#l313

So is it possible that this extension is saving very large amounts of data to the prefservice, or somehow else corrupting prefs or the pref file?

Comment 13

4 years ago
Hmm, nsPrefBranch... This isn't likely to be another casualty of the missed UUID change on nsIPrefBranch, I hope?
That seems pretty unlikely unless crossrider is using a binary component; but I'm pretty sure they are not.
My current theory is that this is actually caused by somebody using the prefserver from an incorrect thread, and is probably only incidentally triggered by extensions asking for prefs at particular times or in certain patterns. This means it would probably be fixed by the things attached to bug 619487. I have no clue how to prove this theory, though, other than wait for those bugs.
Depends on: 619487
I've asked for a copy of the add-on to get QA engaged here for a repro case, given the fact that this remains a top crasher.
Keywords: qawanted, steps-wanted
Created attachment 718097 [details]
Gian Savings XPI

Here it is. We're ready for QA now.
(In reply to Scoobidiver from comment #3)
> Giant Savings (http://giant-savings.com/) displays ad popups (it promises to
> offer you coupon when shopping online).
> Other pieces of adware are not enough correlated to blocklist them.
I can't figure out how to use this addon. 
In addons manager it says: "Save big with Giant Savings! Coupons display instantly while you're shopping online!" 
But I didn't get any notifications on amazon, ebay, sportsdirect and on several other sites resulted from a "shop online coupons" google search.
Any thoughts?
QA Contact: paul.silaghi
My theory from comment 15 is weak because in no case that we know about are we *writing* to the prefservice from another thread; we're only reading from it, which should not cause the pref table to become corrupted.
Any progress on this bug yet? Is there anything more QA can do to assist given that we haven't figured out how this add-on works?
Created attachment 720910 [details] [diff] [review]
Make an infallible AeppendUTF8toUTF16 and clean up nsReadableUtils.cpp, rev. 1

These two patches aren't going to fix anything, but they will save the long or corrupted memory in the minidump.
Attachment #720910 - Flags: review?(justin.lebar+bug)
Comment on attachment 720910 [details] [diff] [review]
Make an infallible AeppendUTF8toUTF16 and clean up nsReadableUtils.cpp, rev. 1

r=me, but you don't have any concerns about shoving a string of potentially unbounded length into crash reports?  Or is the crash reporter service going to take care of this for us?
Attachment #720910 - Flags: review?(justin.lebar+bug) → review+
Created attachment 720917 [details] [diff] [review]
Make an infallible AeppendUTF8toUTF16 and clean up nsReadableUtils.cpp, rev. 1.1

Oops wrong patch
Attachment #720910 - Attachment is obsolete: true
Attachment #720917 - Flags: review?(justin.lebar+bug)
Created attachment 720920 [details] [diff] [review]
Do some custom OOM error reporting in the hopes of catching the relevant memory in the minidump to see what's going on, rev. 1
Attachment #720920 - Flags: review?(khuey)
Attachment #720920 - Flags: review?(ted)
(Reporter)

Updated

4 years ago
Whiteboard: [leave open]
Comment on attachment 720910 [details] [diff] [review]
Make an infallible AeppendUTF8toUTF16 and clean up nsReadableUtils.cpp, rev. 1

You might want to size-check utf8String.Length, since you're stuffing every byte of it into the minidump. Maybe cap it at a few kb?
Attachment #720917 - Flags: review?(justin.lebar+bug) → review+
Attachment #720920 - Flags: review?(ted) → review+
Comment on attachment 720920 [details] [diff] [review]
Do some custom OOM error reporting in the hopes of catching the relevant memory in the minidump to see what's going on, rev. 1

Review of attachment 720920 [details] [diff] [review]:
-----------------------------------------------------------------

I hope this codepath isn't hit very frequently.
Attachment #720920 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/917070d58a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/821013d8066d
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla22
Comment on attachment 720920 [details] [diff] [review]
Do some custom OOM error reporting in the hopes of catching the relevant memory in the minidump to see what's going on, rev. 1

User impact if declined: inability to diagnose crashes
Testing completed (on m-c, etc.): just landed, no real way to test except for users who see the bug
Risk to taking this patch (and alternatives if risky): This should be an OOM-only codepath change, so it should be safe. Doesn't seem especially risky to me.
String or UUID changes made by this patch: None
Attachment #720920 - Flags: approval-mozilla-beta?
Attachment #720917 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/821013d8066d
https://hg.mozilla.org/mozilla-central/rev/917070d58a17

Note that the bug # in Part A was wrong...
Attachment #720917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #720920 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/b0b5a6da2a16
https://hg.mozilla.org/releases/mozilla-beta/rev/6a7999f4cc98

I'm not going to mark status-firefox20:fixed because that's not true.

Comment 32

4 years ago
Note to anyone else doing crash analysis: The patches landed here probably change the crash signatures, possibly to something in pref branch. The crashes do not get fixed by those patches, only hopefully easier to diagnose. We'll have to find out which signatures exactly they move to, and add them to this bug.
Whiteboard: [leave open] → [leave open][no-nag]
(Reporter)

Comment 33

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #32)
> We'll have to find out which signatures exactly they move to, and add them to this bug.
Found because of the comment in App Notes: https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak_P+|+nsPrefBranch%3A%3AGetComplexValue%28char+const*%2C+nsID+const%26%2C+void**%29
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&)] → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
status-firefox21: --- → affected
status-firefox22: --- → affected
Version: 18 Branch → Trunk
From one of those reports:
https://crash-stats.mozilla.com/report/index/cdbd01fe-66f8-4976-871a-80a1a2130312

"bug836263-size": "8016ef0"

That's pretty big (128MB).

I opened this dump up and extracted the saved memory region, and it appears to start with a copy of the jquery script:
/*! jQuery v1.7.1 jquery.com | jquery.org/license */
(function(a,b){function cy(

so, uh...
It's been a couple of releases and we did not get traction here for FF20, wontfixing.
status-firefox20: affected → wontfix
Dropping QAWANTED since we've been unable to find steps to reproduce this crash. Please re-add if the changes landed result in more leads we can follow.
Keywords: qawanted
I retested this on the following URLs:
www.1-800-bakery.com
www.1800flowers.com

I get a green Giant Savings widget in the top right on page load which displays some coupons on hover. I tried applying various coupons and playing around with the settings but I was not able to reproduce a crash. I tested with Firefox Nightly 22.0a1.
Some time ago I filed Bug 511135. Whether it has any relevance here I don't know, but I will offer that the frequency of that crash has increased for me in recent months perhaps in the same time frame as this issue would have started on trunk. Like this bug, I don't know why the increase in occurrences. Note: my crashes related to Bug 511135 get reported by breakpad as "Empty ..."
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> From one of those reports:
> https://crash-stats.mozilla.com/report/index/cdbd01fe-66f8-4976-871a-
> 80a1a2130312
> 
> "bug836263-size": "8016ef0"
> 
> That's pretty big (128MB).
> 
> I opened this dump up and extracted the saved memory region, and it appears
> to start with a copy of the jquery script:
> /*! jQuery v1.7.1 jquery.com | jquery.org/license */
> (function(a,b){function cy(
> 
> so, uh...

Would it be possible to look into more of these? Partner/QA investigations are not producing anything interesting, so we're really just left with the extra diagnostics we introduced.
Flags: needinfo?(ted)
Flags: needinfo?(benjamin)
At this point, I don't have the bandwidth to look at more of these if I'm going to deal with bug 837835 at all.
Assignee: benjamin → nobody
Flags: needinfo?(benjamin)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> From one of those reports:
> https://crash-stats.mozilla.com/report/index/cdbd01fe-66f8-4976-871a-
> 80a1a2130312
> 
> "bug836263-size": "8016ef0"
> 
> That's pretty big (128MB).
> 
> I opened this dump up and extracted the saved memory region, and it appears
> to start with a copy of the jquery script:
> /*! jQuery v1.7.1 jquery.com | jquery.org/license */
> (function(a,b){function cy(
> 
> so, uh...

I pulled several dumps and they all have this.
Flags: needinfo?(ted)
Created attachment 732503 [details] [diff] [review]
Moar diagnostics

Lets get the pref name.
Attachment #732503 - Flags: review?(benjamin)
Attachment #732503 - Flags: review?(benjamin) → review+
Comment on attachment 732503 [details] [diff] [review]
Moar diagnostics

I'd like to land all three patches on beta-21 so we can get more insight here.
Attachment #732503 - Flags: approval-mozilla-beta?
It looks like the Deals Plugin extension, which goes by crossriderapp4637@crossrider.com in https://crash-stats.mozilla.com/report/index/cdbd01fe-66f8-4976-871a-80a1a2130312 and extension21806@extension21806.com at dealsplugin.com, is the culprit.

As far as I can tell, this is a horror show, with a version of the Crossrider framework which fetches a configuration file from https://w9u6a2p6.ssl.hwcdn.net/plugin/apps/21806/manifest/091/ff/manifest.xml, which it uses to fetch another configuration file from http://app-static.crossrider.com/plugin/apps/21806/plugins/091/ff/plugins.json, from which it downloads a list of JS files, which it saves to preferences, and then later executes (incidentally opening a hell of a MITM attack...)
So do we have contacts with the right people?  Can we get them to fix it based on the information in comment 44?
Flags: needinfo?(akeybl)
Presumably we could just blocklist this version of this extension as well, since it's horribly broken?
Alternately, maybe we could just let these get*Pref methods fail in these cases? JS callers should already be handling exceptions since get*Pref can throw if the pref is unset, right?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #46)
> Presumably we could just blocklist this version of this extension as well,
> since it's horribly broken?

Unfortunately, as far as I can tell it's the framework that's at fault, so blocklisting one extension probably wouldn't help much.
So, is this problem present in all Crossrider extensions? What is different about the Deals Plugin extension?
This is different than the framework in the add-ons they submit to AMO. I'm not sure if it's a newer version, or they've just been packaging add-ons different when they export them for AMO, but the Crossrider add-ons they've submitted to us do not do this. It's pretty clear that this code is part of the framework, though.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #47)
> Alternately, maybe we could just let these get*Pref methods fail in these
> cases? JS callers should already be handling exceptions since get*Pref can
> throw if the pref is unset, right?

I don't think so. Most extensions use getPrefType to decide which method to call, and the rest tend to just set it in the default branch assume it's set if they expect it to be.

Either way, I think throwing rather than crashing is the right way to handle it, at least when the strings we're talking about are this large.

Comment 52

4 years ago
From all I've heard about those add-ons and about what the framework seems to do here, I'm not a friend of them and blocklisting sounds good to me if they are abusing our infrastructure (including the pref service).

That said, I think if add-ons do seemingly crazy things like this, we should not crash but fail in a different way if possible. If that makes the add-on not work, that's OK, the user will notice that and uninstall that (and the devs will see what fails and be able to work on correcting it - getting an error/exception on a call from their code makes it way more debuggable from their side, actually). If the whole Firefox instance goes away crashing, that's bad.

So, the "throw instead of crash" from comment #51 sounds to me like a good solution to me. :)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #52)
> From all I've heard about those add-ons and about what the framework seems
> to do here, I'm not a friend of them and blocklisting sounds good to me if
> they are abusing our infrastructure (including the pref service).

I don't think that blocklisting is an option. Even if we used a regexp block
for the two known ID patterns above, that wouldn't cover them all. There's
also the problem that there are non-foistware add-ons using versions of the
Crossrider framework without this issue. It may be necessary to blocklist the
add-ons with the highest correlations with this crash signature, though.

In any case, I think we should see how the developers respond, and put
blocklisting back on the table if it doesn't look like they can address it in
a reasonable time frame.
I just forwarded the new information to Crossrider.
I had another look at the framework submitted to AMO and it *is* the same as in this add-on, but the code that fetches remote scripts is prefed off. Scripts are still fetched from within the XPI and stored in preferences, though. In any case, we haven't actually accepted any add-ons using this framework on AMO, so the point is academic.

Updated

4 years ago
Attachment #732503 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kris Maglione [:kmag] from comment #53)
> It may be necessary to blocklist the add-ons with the highest
> correlations with this crash signature, though.

Hm. This is now the #13 top crasher in 20 and the comments indicate that the
crashes happen often. We should probably do this sooner rather than later.
(Reporter)

Updated

4 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
(Reporter)

Updated

4 years ago
Depends on: 858791
(Reporter)

Updated

4 years ago
Depends on: 858926

Comment 57

4 years ago
We discussed this in today's stability meeting shortly, and both me and bsmedberg agreed that we should do someth9ing on our side here. It makes sense to limit the size of prefs being set and error out for huge ones instead of crashing.

Can we please get a patch for doing that?
back to khuey
Assignee: nobody → khuey
Component: XPCOM → Preferences: Backend
(Reporter)

Updated

4 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
(Reporter)

Updated

4 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
Created attachment 743345 [details] [diff] [review]
Possible Patch

Something like this perhaps?
Attachment #718097 - Attachment is obsolete: true
Attachment #720917 - Attachment is obsolete: true
Attachment #720920 - Attachment is obsolete: true
Attachment #732503 - Attachment is obsolete: true
Attachment #743345 - Flags: review?(benjamin)
Comment on attachment 743345 [details] [diff] [review]
Possible Patch

Yeah, that should work. I was going to say 10k, though... thoughts?
Attachment #743345 - Flags: review?(benjamin) → review+
I think how aggressive we should be depends on what branch we want to land this on.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #60)
> Yeah, that should work. I was going to say 10k, though... thoughts?

kris%nawk '{ m = length($0); if (m > n) n = m } END { print n }' <prefs.js
51723

That's NoScript's CAPS site list. I checked a few other profiles I have floating around, and the smallest is 15K.

Comment 63

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #61)
> I think how aggressive we should be depends on what branch we want to land
> this on.

Well, I'm happy with this riding the trains, as the original crash has been fixed with add-on updates anyhow.

The original crash here seems to be in the range of 128M for the string, see comment #34, and as Kris says in comment #62, NoScript seems to store a rather large (but not crashing) string with 15-51k, so we can either go for the 1MB in the existing patch, go down much further like bsmedberg suggests in comment #60 and message strongly to add-on authors that they need to fix any add-on that uses prefs that potentially are larger than the limit, or go with something in between (say, 100k or similar) and try to avoid having problems with any other add-ons but still limit to lower than an MB.

That said, we need to have this messaged in our "add-on-relevant changes in Firefox 23" section anyhow (what flag/keyword to set for that?). Not sure if it should even make relnotes or not.
relnote-firefox: --- → ?
I support going with a high limit, like 1MB or something in the hundreds of Kb, to minimize add-on breakage.
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/2e46cabb6a11
I don't know if there's anything left to do here, but if there is I'm the wrong owner.
Assignee: khuey → nobody
Status: ASSIGNED → NEW

Comment 67

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #66)
> I don't know if there's anything left to do here, but if there is I'm the
> wrong owner.

I think it should stay assigned to you and get marked FIXED as what we can do on our side has been done. No idea why Scoobidiver has set a "[leave open]" here.

IIRC, we discussed in the last channel meetings that we probably should uplift this one to 22 as long as it's still on Aurora, so this is something you still can do here. :)
(Reporter)

Comment 68

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #67)
> No idea why Scoobidiver has set a "[leave open]" here.
Based on "Do some custom OOM error reporting in the hopes of catching the relevant memory in the minidump to see what's going on, rev. 1" in comment 24.
It was not about the patch that landed in comment 65 but nevertheless it's still applicable as there are still crashes: https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A23.0a1&signature=mozalloc_abort%28char%20const*%20const%29%20|%20NS_DebugBreak%20|%20nsPrefBranch%3A%3AGetComplexValue%28char%20const*%2C%20nsID%20const%26%2C%20void**%29
Summary: crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\dist\include\nsTSubstring.h, line 529" mainly with Giant Savings → crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\...\obj-firefox\dist\include\nsTSubstring.h, line 533 or e:/.../build/modules/libpref/src/nsPrefBranch.cpp, line 330" mainly with Giant Savings

Comment 69

4 years ago
(In reply to Scoobidiver from comment #68)
> there are still crashes

Indeed. Ouch. :(
Well the changes I made only affect writes, not reads, so if you already have giant prefs it won't save you.

Also we should probably back out http://hg.mozilla.org/mozilla-central/rev/821013d8066d

Comment 71

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #70)
> Well the changes I made only affect writes, not reads, so if you already
> have giant prefs it won't save you.

Ah, makes sense.

> Also we should probably back out
> http://hg.mozilla.org/mozilla-central/rev/821013d8066d

Well, I guess it makes sense to crash in nsPrefBranch::GetComplexValue instead of AppendUTFXXtoUTFYY but the debugging code mentioning this bug should probably go away. Benjamin?

Comment 72

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #71)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #70)
> > Well the changes I made only affect writes, not reads, so if you already
> > have giant prefs it won't save you.
> 
> Ah, makes sense.

That said, I wonder if we should care to clean up somehow as I guess having 128MB+ in prefs might be a significant perf hit when we read prefs as well.
(Reporter)

Comment 73

4 years ago
In 21.0b6, it's a new signature: https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak_P+|+nsAString_internal%3A%3AGetMutableData%28wchar_t**%2C+unsigned+int%29
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
Jorge typically blogs about addon-compat changes, so no need to relnote unless we know of regressions ahead of release.
relnote-firefox: ? → -
Flags: needinfo?(akeybl) → needinfo?(jorge)
Yes, this is already flagged for addon-compat, so I'll take care of it when it's closer to release.
Flags: needinfo?(jorge)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 76

4 years ago
Kyle, can you still request uplift to Aurora 22? IIRC, Jorge said that with the 1MB limit we still should be OK to do this there.
Assignee: nobody → khuey
(Reporter)

Comment 77

4 years ago
It's currently only #72 browser crasher in 21.0b7 (at least no behind empty dump crashes) while it was #13 in 21.0b6, #6 in 21.0b5, #17 in 21.0b3, #15 in 21.0b2, and #17 in 21.0b1 over the last four weeks..
The Beta working range is:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=c1453860aef9&tochange=0fe6378ac802
Whiteboard: [leave open][no-nag] → [no-nag]

Comment 78

4 years ago
Yes, it looks like a fixed add-on has broadly been pushed this week which made this disappear from topcrash views (if you look at the last two days, it's below #150 or so even in release, so any "working range" in terms of our code is a false positive, the fix is on the add-on side).
(Reporter)

Comment 79

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #78)
> Yes, it looks like a fixed add-on has broadly been pushed this week
A fix for bug 867342 would have avoided my mistake.
(Reporter)

Comment 80

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #78)
> Yes, it looks like a fixed add-on has broadly been pushed this week
Correlations are now fixed and there's indeed a new version of Giant Saving, 0.91.97, with accounted for about 50% of crashes and that no longer crashes:
  mozalloc_abort(char const* const) | NS_DebugBreak_P | nsPrefBranch::GetComplexValue(char const*, nsID const&, void**)|EXCEPTION_BREAKPOINT (43 crashes)
     49% (21/43) vs.   0% (64/136061) crossriderapp13370@crossrider.com (PhotoMania)
          7% (3/43) vs.   0% (8/136061) 0.91.136
         42% (18/43) vs.   0% (55/136061) 0.91.142
     16% (7/43) vs.   0% (138/136061) crossriderapp4479@crossrider.com (GiantSaving)
          7% (3/43) vs.   0% (5/136061) 0.91.90
          7% (3/43) vs.   0% (10/136061) 0.91.92
          2% (1/43) vs.   0% (3/136061) 0.91.95
          0% (0/43) vs.   0% (39/136061) 0.91.96
          0% (0/43) vs.   0% (80/136061) 0.91.97
     19% (8/43) vs.   3% (4162/136061) plugin@yontoo.com (Yontoo Layers)
         19% (8/43) vs.   3% (4121/136061) 1.20.02
     12% (5/43) vs.   0% (35/136061) crossriderapp4352@crossrider.com (CouponDropDown)
          5% (2/43) vs.   0% (24/136061) 0.91.100
          7% (3/43) vs.   0% (9/136061) 0.91.92
          0% (0/43) vs.   0% (1/136061) 0.91.94
     12% (5/43) vs.   5% (6419/136061) {635abd67-4fe9-1b23-4f01-e679fa7484c1} (Yahoo! Toolbar, https://addons.mozilla.org/addon/2032)
         12% (5/43) vs.   4% (6059/136061) 2.5.9.20130409112616
          0% (0/43) vs.   0% (124/136061) 2.5.9.20130411104515
          0% (0/43) vs.   0% (113/136061) 2.5.9.20130418100420
          0% (0/43) vs.   0% (4/136061) 2.6.0.20130418072822
          0% (0/43) vs.   0% (8/136061) 3.0.2.20121108061959
(Reporter)

Updated

4 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
OS: Windows 7 → All
Hardware: x86 → All
Summary: crash in AppendUTF8toUTF16 or AppendUTF16toUTF8 with abort message: "OOM: file e:\...\obj-firefox\dist\include\nsTSubstring.h, line 533 or e:/.../build/modules/libpref/src/nsPrefBranch.cpp, line 330" mainly with Giant Savings → crash in AppendUTF<n>toUTF<m> with abort message: "OOM: file e:\...\nsTSubstring.h, line 533" or in nsPrefBranch::GetComplexValue with abort message: "e:/.../modules/libpref/src/nsPrefBranch.cpp, line 330" mainly with Giant Savings
Target Milestone: mozilla22 → mozilla23
(Reporter)

Updated

4 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip; → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AppendUTF16toUTF8(nsAString_internal const&, nsACString_internal&&hellip;
Kyle, does anything need doing for the branches?
(In reply to Robert Kaiser (:kairo@mozilla.com) [away until early June] from comment #76)
> Kyle, can you still request uplift to Aurora 22? IIRC, Jorge said that with
> the 1MB limit we still should be OK to do this there.

I don't really feel comfortable with pushing this to 22.  I think we should get as much testing as we can on this before releasing.
Setting status flags per comment 82. Kyle, please change if I misunderstood.
status-firefox18: affected → ---
status-firefox19: affected → ---
status-firefox21: affected → wontfix
status-firefox22: affected → wontfix
status-firefox23: --- → fixed
Is there anything needed from QA on this now that this is "fixed" in Beta?
Checking the crash stats, I see no crashes on FF 23. I guess we can call this bug verified.
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
I found my way to this code while researching the RegisterAppMemory function.

Apparently this crash still happens in low volume on 40 and 41. Here's an example with size 0x75406f8: bp-92bd627b-8cc0-4f9e-acfa-90f1b2151005.

I was unable to find the contents of the string, because the address of utf8String.mData wasn't recorded in the report, and the register containing that value had been clobbered already (oops).
You need to log in before you can comment on or make changes to this bug.