Paul Bone [:pbone] Assignee
	Description • 7 years ago

Add a temporary conary field to nsStringBuffer to help root out suspected heap crashes, bug 1405525.

	Bobby Holley (:bholley)
	Comment 1 • 7 years ago

Ping. See bug 1406996 comment 37 for why we should get moving with this.

	Paul Bone [:pbone] Assignee
	Comment 2 • 7 years ago

(In reply to Bobby Holley (parental leave - send mail for anything urgent) from comment #1)
> Ping. See bug 1406996 comment 37 for why we should get moving with this.

Thanks. I'll make this my current task.

	Paul Bone [:pbone] Assignee
	Comment 3 • 7 years ago

Hi bz, you were suggested as a reviewer for this patch.  We want to add a canary value to nsStringBuffer and have it in nightly for a few days to try to detect suspected heap curruption.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab2a107d0312ca4acbd8e6c0816eaae5b69bab11

Thanks.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 7 years ago

Comment on attachment 8921862 [details] [diff] [review]
Bug 1410276 - Add a canary field to nsStringBuffer

You probably don't want to include the trailing ';' in the CHECK_STRING_BUFFER_CANARY macro (both versions).

Making FromData out of line might be a perf hit that we notice in some cases, but that's OK for a temporary thing.

	Paul Bone [:pbone] Assignee
	Comment 5 • 7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Comment on attachment 8921862 [details] [diff] [review]
> Bug 1410276 - Add a canary field to nsStringBuffer
> 
> You probably don't want to include the trailing ';' in the
> CHECK_STRING_BUFFER_CANARY macro (both versions).

Derp.

> Making FromData out of line might be a perf hit that we notice in some
> cases, but that's OK for a temporary thing.

I thought so too, I'll add a comment for anyone else reading this.

FWIW I wrote this in a way that we can keep it in if we chose to.  If we do that we could remove the assertion in this function and move it back to the class definition.

Thanks.

	Paul Bone [:pbone] Assignee
	Comment 6 • 7 years ago

r+ carried forward.

	Pulsebot
	Comment 7 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3785ec9a48c
Add a canary field to nsStringBuffer. r=bz

	Attila Craciun [:arny]
	Comment 8 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a3785ec9a48c

	Paul Bone [:pbone] Assignee
	Comment 9 • 7 years ago

This seems to have cought a few crashes, I might need some guideance on how to get the dump out of the crash reports and somehow examine it:

https://crash-stats.mozilla.com/search/?signature=%3Dmozalloc_abort%20%7C%20NS_DebugBreak%20%7C%20nsStringBuffer%3A%3AFromData&signature=%3Dmozalloc_abort%20%7C%20Abort%20%7C%20NS_DebugBreak%20%7C%20nsStringBuffer%3A%3ARelease&version=58.0a1&date=%3E%3D2017-10-25T14%3A01%3A00.000Z&date=%3C2017-11-14T14%3A01%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

	Chris Peterson [:cpeterson]
	Comment 10 • 7 years ago

Emilio, do you have access to the crash report dumps?

	Emilio Cobos Álvarez (:emilio)
	Comment 11 • 7 years ago

Yes, I do. Paul, let me know what I should look for, or reach to me on IRC so we can talk about it or what not.

Though probably you should get access to the dumps given you're working on the GC...

	Andrew McCreight [:mccr8]
	Comment 12 • 7 years ago

The "moz crash reason" field for all of these crashes don't contain any useful information. These are all content process crashes, and in the content process NS_RUNTIMEABORT  doesn't do anything: http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/xpcom/base/nsDebugImpl.cpp#394

	Andrew McCreight [:mccr8]
	Comment 13 • 7 years ago

Well, of course it crashes. I guess the actual field for this would be AbortMessage but that is blank, too.

	Andrew McCreight [:mccr8]
	Comment 14 • 7 years ago

Bug 1412048 replaced some uses of NS_RUNTIMEABORT, but it looks like it missed this one. But you can just land a patch to do a similar transformation for your call site.

	Paul Bone [:pbone] Assignee
	Comment 15 • 7 years ago

mcrr8:

When I read that other bug it sounds like all the relevant data _is_ captured, it's just not public.  So hopefully having access to the full crash reports will help me.

All:

I have applied for minidump access and should be able to access this shortly.

I noticed that all the crashes in Release() are on windows and all the crashes in FromData() are on Linux.  This is probably just a coincidence: there are only 8 crashes in total.

	Andrew McCreight [:mccr8]
	Comment 16 • 7 years ago

Well, that comment isn't quite right. It only applies to annotations in the parent process, AFAICT. I have minidump access and I couldn't see them. It looks like a more recent patch did update the canary annotation so these should start showing up: https://hg.mozilla.org/mozilla-central/rev/e85f59ea455d

	Paul Bone [:pbone] Assignee
	Comment 17 • 7 years ago

Okay thanks.

I expect that with a minidump I'll be able to inspect the memory and find out what these values were.

	Chris Peterson [:cpeterson]
	Comment 18 • 7 years ago

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Bug 1412048 replaced some uses of NS_RUNTIMEABORT, but it looks like it
> missed this one. But you can just land a patch to do a similar
> transformation for your call site.

That is not necessary. When I landed the patch for bug 1412048 that removed NS_RUNTIMEABORT last week, I replaced CHECK_STRING_BUFFER_CANARY's NS_RUNTIMEABORT call with MOZ_CRASH_UNSAFE_PRINTF:

https://searchfox.org/mozilla-central/diff/4a8bdd4ca5087c07464aaab990dfc3ccd4d3c909/xpcom/string/nsSubstring.cpp#44

	Paul Bone [:pbone] Assignee
	Comment 19 • 7 years ago

Backed out in inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/23f86a1ac424

	Paul Bone [:pbone] Assignee
	Comment 20 • 7 years ago

Backed out in central: https://hg.mozilla.org/mozilla-central/rev/23f86a1ac424

	Paul Bone [:pbone] Assignee
	Comment 21 • 7 years ago

Hi bz,

The original patch was backed out because 58 is getting close to beta.  But it hasn't caught many assertions yet.  This patch should affect performance less and might be valuable.  Is this worth persuing or should we close this bug?

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 7 years ago

Comment on attachment 8926718 [details] [diff] [review]
Bug 1410276 - Add a canary field to nsStringBuffer

This seems fine, though FromDataOutlineCheckCanary is only called when the check would fail, so we could just rename it to FromDataCanaryCheckFailed and nix the checks inside it.  Either way, we should document what it does.

	Paul Bone [:pbone] Assignee
	Comment 23 • 7 years ago

Updated based on review feedback. r+ carried forward.

	Paul Bone [:pbone] Assignee
	Comment 24 • 7 years ago

Here are some talos jobs to help determine what affect this has on performance and help us decide if it's worth committing.  Do we want this to go to Beta with 58 assuming there's no affect on performance?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6a49fa7d322ed4e8acaf6f7556452659157b9a
Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=6b6a49fa7d322ed4e8acaf6f7556452659157b9a

NI: jonco, I'm not sure who else to ask and this relates to  Bug 1405525

	Jon Coppeard (:jonco)
	Comment 25 • 7 years ago

(In reply to Paul Bone [:pbone] from comment #24)
If there's no impact on performance I'd say let's put this in and see what it turns up.

	Paul Bone [:pbone] Assignee
	Comment 26 • 7 years ago

The latest patch causes a single 4.82% perf regression for tp5o_webext responsiveness opt e10s.  I only bothered to test on linux64, there shouldn't be any platform differences.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=e2f87726b608&newProject=try&newRevision=6b6a49fa7d322ed4e8acaf6f7556452659157b9a&framework=1&showOnlyImportant=1

I'm going to wait until 58 goes to beta and then push this on 59 for a while.

	Pulsebot
	Comment 27 • 7 years ago

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced8af085927
Add a canary field to nsStringBuffer r=bz

	Paul Bone [:pbone] Assignee
	Comment 28 • 7 years ago

I've committed this again so that it can run for a while on 59.

Ionuț this will likely cause some perf regressions, like last time but hopefully fewer / less sagnificant.

Thanks.

	Tiberius Oros[:tiberius_oros]
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ced8af085927

	Ionuț Goldan [:igoldan]
	Comment 30 • 7 years ago

(In reply to Paul Bone [:pbone] from comment #28)
> I've committed this again so that it can run for a while on 59.
> 
> Ionuț this will likely cause some perf regressions, like last time but
> hopefully fewer / less sagnificant.
> 
> Thanks.

Thanks for the heads up!

	Marcia Knous [:marcia]
	Comment 31 • 7 years ago

Here are a few Mac nightly crashes in 59 that I spotted today while going through crash stats: http://bit.ly/2iWsYBg

	Chris Peterson [:cpeterson]
	Comment 32 • 7 years ago

Paul, any updates on this nsStringBuffer canary? It's still enabled in Nightly 60 and there are about a dozen crash reports from the last week. Should we remove this code or just leave it?

https://crash-stats.mozilla.com/search/?signature=~FromDataCanaryCheckFailed&product=Firefox&date=%3E%3D2018-02-03T23%3A47%3A50.000Z&date=%3C2018-02-10T23%3A47%3A50.000Z&_sort=-date&_facets=signature&_facets=cpu_arch&_facets=version&_facets=platform_pretty_version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

	Paul Bone [:pbone] Assignee
	Comment 33 • 7 years ago

Oh,

I'll take a look at those crashes later today as Bug 1405525.

I lost track of this bug and didn't realise it was still providing crashes.  I'm happy to keep this code in since it's only used in Nightly and Debug builds.  I'll close this bug and continue in the original Bug 1405525.