Closed Bug 1256541 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s ? ---
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

Attachments

(1 file)

This is why bug 1236108 is not merging to aurora or beta cleanly.
Blocks: 1236108
Blocks: 1257486
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/
Attachment #8731785 - Flags: review?(benjamin)
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
Attachment #8731785 - Flags: review?(benjamin) → review+
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.
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
I made some false assumptions with that assertion that was firing. I removed it as the code can safely deal with that case.
Flags: needinfo?(aklotz)
https://hg.mozilla.org/mozilla-central/rev/faf8710ec829
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Can you request uplift? Then we can also uplift bug 1236108. :)
Flags: needinfo?(aklotz)
Depends on: 1259809
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
Flags: needinfo?(aklotz)
Attachment #8731785 - Flags: approval-mozilla-beta?
Attachment #8731785 - Flags: approval-mozilla-aurora?
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
Attachment #8731785 - Flags: approval-mozilla-beta?
Attachment #8731785 - Flags: approval-mozilla-beta+
Attachment #8731785 - Flags: approval-mozilla-aurora?
Attachment #8731785 - Flags: approval-mozilla-aurora+
Hitting conflicts on both aurora and beta.
Flags: needinfo?(aklotz)
I need to land this myself.
Flags: needinfo?(aklotz)
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'
Flags: needinfo?(aklotz)
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);
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.
Flags: needinfo?(aklotz)
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
Depends on: 1270018
You need to log in before you can comment on or make changes to this bug.