1256541 - Exception-time crash annotations using incorrect paths when MOZ_CONTENT_SANDBOX is not defined

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

This is why bug 1236108 is not merging to aurora or beta cleanly.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: MozReview Request: Bug 1256541: Fix incorrect generation of path for child process .extra files when content sandboxing is disabled; r?bsmedberg

Since the minidump path can be overridden programmatically in the chrome
process, using that path as the base for .extra files won't work since content
is unaware of it.

This patch changes everything to use the temp path when MOZ_CONTENT_SANDBOX is
not defined or when sandboxing is disabled via pref. It also moves the derivation
of the content temp path out of exception context on Windows and Mac, as I
found out that those functions touch the heap.

I also noticed that xpcshell is not sandbox-aware when utilized as a parent
process. I've filed bug 1257098 to take care of that, but this patch includes a
hack for the immediate term.

Review commit: https://reviewboard.mozilla.org/r/40833/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40833/

Comment 3

[Benjamin Smedberg]

Comment on attachment 8731785 [details]
MozReview Request: Bug 1256541: Fix incorrect generation of path for child process .extra files when content sandboxing is disabled; r?bsmedberg

https://reviewboard.mozilla.org/r/40833/#review38747

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0827e0e5d9d4

Comment 5

[Andrew McCreight [:mccr8]]

Thanks for fixing this, Aaron. I've marked it blocking all of the OOM | Unknowns we have that aren't related to the JS engine.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0974ea6ae83b for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=24573582&repo=mozilla-inbound

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faf8710ec82947959292a72c11790a340f70b083
Bug 1256541: Fix incorrect generation of path for child process .extra files when content sandboxing is disabled; r=bsmedberg

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

I made some false assumptions with that assertion that was firing. I removed it as the code can safely deal with that case.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift? Then we can also uplift bug 1236108. :)

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8731785 [details]
MozReview Request: Bug 1256541: Fix incorrect generation of path for child process .extra files when content sandboxing is disabled; r?bsmedberg

(Sheriff note: this needs to be landed by aklotz)

Approval Request Comment
[Feature/regressing bug #]: Crash reporter OOM annotations for e10s content processes
[User impact if declined]: Ability to analyze content OOM crashes would be impaired
[Describe test coverage new/current, TreeHerder]: Existing tests in xpcshell and browser-chrome
[Risks and why]: Low, this patch simplifies existing code which is mostly related to calculation of target directories for crash dump annotations.
[String/UUID change made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8731785 [details]
MozReview Request: Bug 1256541: Fix incorrect generation of path for child process .extra files when content sandboxing is disabled; r?bsmedberg

Useful for e10s crash reports. Please uplift to aurora and beta

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hitting conflicts on both aurora and beta.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

I need to land this myself.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/releases/mozilla-aurora/rev/044150f51647 
https://hg.mozilla.org/releases/mozilla-beta/rev/72fb48644f42

Comment 16

[Frank-Rainer Grahl (:frg)]

I think this one just broke VS2015 compilation in the beta tree:

c:/seamonkey/comm-beta/mozilla/toolkit/crashreporter/nsExceptionHandler.cpp(1198): error C2664: 'size_t CrashReporter::BuildTempPath
(wchar_t *,size_t)': cannot convert argument 1 from 'nsAString_internal::char_iterator' to 'wchar_t *'mozmake[7]: Entering directory
 'c:/seabuild/release/comm-beta-15/obj-x86_64-pc-mingw32/intl/icu/target/tools/gencmn'

Comment 17

[Frank-Rainer Grahl (:frg)]

On aurora too.

c:/seamonkey/comm-aurora/mozilla/toolkit/crashreporter/nsExceptionHandler.cpp(1215): error C2664: 'size_t CrashReporter::BuildTempPath(wchar_t *,size_t)': cannot convert argument 1 from 'nsAString_internal::char_iterator' to 'wchar_t *'
c:/seamonkey/comm-aurora/mozilla/toolkit/crashreporter/nsExceptionHandler.cpp(1215): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

wwc fixed it:

>> size_t actualLen = BuildTempPath(wwc(aResult.BeginWriting()), XP_PATH_MAX);

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Feel free to file a follow-up bug to fix this if you like, but since our official VC2015 support is effective for Version 48, I'm not personally inclined to request uplift for this change.

Comment 19

[Frank-Rainer Grahl (:frg)]

Doesn't make much sense to file a bug if it's likely not being checked in. Will just put private fixes in my trees. 

Thanks for your time
FRG