836263 - 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

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Scoobidiver (away)]

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) [e-mail] (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) [e-mail] (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) [e-mail] (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

Comment 1

[Scoobidiver (away)]

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) [e-mail] (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) [e-mail] (1.20.00)
     15% (167/1133) vs.   0% (408/204664) [e-mail] (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) [e-mail]
          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) [e-mail]
          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) [e-mail] (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) [e-mail]
          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) [e-mail] (1.20.00)

Comment 2

[Alex Keybl [:akeybl]]

Jorge - how do you feel about blocking these malware add-ons? Have we done so in the past?

Comment 3

[Scoobidiver (away)]

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.

Comment 4

[Scoobidiver (away)]

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

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

(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

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 7

[Robert Kaiser]

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.

Comment 8

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) 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) [e-mail]
          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) [e-mail]
          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) [e-mail]
          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

Comment 9

[Robert Kaiser]

(In reply to Scoobidiver from comment #8)
> (In reply to Robert Kaiser (:[e-mail]) 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.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Because we don't know yet if this requires work on our end, leaving it nominated for tracking while more info is gathered.

Comment 11

[Alex Keybl [:akeybl]]

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.

Comment 12

[Benjamin Smedberg]

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

[Robert Kaiser]

Hmm, nsPrefBranch... This isn't likely to be another casualty of the missed UUID change on nsIPrefBranch, I hope?

Comment 14

[Benjamin Smedberg]

That seems pretty unlikely unless crossrider is using a binary component; but I'm pretty sure they are not.

Comment 15

[Benjamin Smedberg]

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.

Comment 16

[Alex Keybl [:akeybl]]

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.

Comment 17

[Alex Keybl [:akeybl]]

Attachment: Gian Savings XPI

Here it is. We're ready for QA now.

Comment 18

[Paul Silaghi, QA [:pauly]]

(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?

Comment 19

[Benjamin Smedberg]

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.

Comment 20

[u279076]

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?

Comment 21

[Benjamin Smedberg]

Attachment: 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.

Comment 22

[Justin Lebar (not reading bugmail)]

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?

Comment 23

[Benjamin Smedberg]

Attachment: Make an infallible AeppendUTF8toUTF16 and clean up nsReadableUtils.cpp, rev. 1.1

Oops wrong patch

Comment 24

[Benjamin Smedberg]

Attachment: 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

Comment 25

[(not currently active) Ted Mielczarek]

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?

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 27

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/917070d58a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/821013d8066d

Comment 28

[Benjamin Smedberg]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/821013d8066d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/917070d58a17

Note that the bug # in Part A was wrong...

Comment 31

[Benjamin Smedberg]

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

[Robert Kaiser]

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.

Comment 33

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) 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

Comment 34

[(not currently active) Ted Mielczarek]

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...

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

It's been a couple of releases and we did not get traction here for FF20, wontfixing.

Comment 36

[u279076]

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.

Comment 37

[u279076]

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.

Comment 38

[Wayne Mery (:wsmwk)]

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 ..."

Comment 39

[Alex Keybl [:akeybl]]

(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.

Comment 40

[Benjamin Smedberg]

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.

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Moar diagnostics

Lets get the pref name.

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 44

[Kris Maglione [:kmag]]

It looks like the Deals Plugin extension, which goes by [e-mail] in https://crash-stats.mozilla.com/report/index/cdbd01fe-66f8-4976-871a-80a1a2130312 and [e-mail] 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...)

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So do we have contacts with the right people?  Can we get them to fix it based on the information in comment 44?

Comment 46

[(not currently active) Ted Mielczarek]

Presumably we could just blocklist this version of this extension as well, since it's horribly broken?

Comment 47

[(not currently active) Ted Mielczarek]

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?

Comment 48

[Kris Maglione [:kmag]]

(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.

Comment 49

[Jorge Villalobos [:jorgev] (he/him)]

So, is this problem present in all Crossrider extensions? What is different about the Deals Plugin extension?

Comment 50

[Kris Maglione [:kmag]]

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.

Comment 51

[Kris Maglione [:kmag]]

(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

[Robert Kaiser]

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. :)

Comment 53

[Kris Maglione [:kmag]]

(In reply to Robert Kaiser (:[e-mail]) 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.

Comment 54

[Jorge Villalobos [:jorgev] (he/him)]

I just forwarded the new information to Crossrider.

Comment 55

[Kris Maglione [:kmag]]

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.

Comment 56

[Kris Maglione [:kmag]]

(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.

Comment 57

[Robert Kaiser]

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?

Comment 58

[Benjamin Smedberg]

back to khuey

Comment 59

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Possible Patch

Something like this perhaps?

Comment 60

[Benjamin Smedberg]

Comment on attachment 743345 [details] [diff] [review]
Possible Patch

Yeah, that should work. I was going to say 10k, though... thoughts?

Comment 61

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think how aggressive we should be depends on what branch we want to land this on.

Comment 62

[Kris Maglione [:kmag]]

(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

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 64

[Jorge Villalobos [:jorgev] (he/him)]

I support going with a high limit, like 1MB or something in the hundreds of Kb, to minimize add-on breakage.

Comment 65

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2e46cabb6a11

Comment 66

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't know if there's anything left to do here, but if there is I'm the wrong owner.

Comment 67

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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. :)

Comment 68

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) 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

Comment 69

[Robert Kaiser]

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

Indeed. Ouch. :(

Comment 70

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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

[Robert Kaiser]

(In reply to Robert Kaiser (:[e-mail]) from comment #71)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 73

[Scoobidiver (away)]

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

Comment 74

[Alex Keybl [:akeybl]]

Jorge typically blogs about addon-compat changes, so no need to relnote unless we know of regressions ahead of release.

Comment 75

[Jorge Villalobos [:jorgev] (he/him)]

Yes, this is already flagged for addon-compat, so I'll take care of it when it's closer to release.

Comment 76

[Robert Kaiser]

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.

Comment 77

[Scoobidiver (away)]

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

Comment 78

[Robert Kaiser]

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).

Comment 79

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) 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.

Comment 80

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) 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) [e-mail] (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) [e-mail] (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) [e-mail] (Yontoo Layers)
         19% (8/43) vs.   3% (4121/136061) 1.20.02
     12% (5/43) vs.   0% (35/136061) [e-mail] (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

Comment 81

[Ryan VanderMeulen [:RyanVM]]

Kyle, does anything need doing for the branches?

Comment 82

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Robert Kaiser (:[e-mail]) [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.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

Setting status flags per comment 82. Kyle, please change if I misunderstood.

Comment 84

[u279076]

Is there anything needed from QA on this now that this is "fixed" in Beta?

Comment 85

[Paul Silaghi, QA [:pauly]]

Checking the crash stats, I see no crashes on FF 23. I guess we can call this bug verified.

Comment 86

[(Away)]

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).