No more #ifdef MOZ_CRASHREPORTER directives needed when using crash reporter functions
[cross-posting to firefox-dev]
Fellow mozillians,
bug 1402519 [1] just landed and it introduces a dummy implementation of
the crash reporter which is built when configuring with
--disable-crash-reporter. This removes the need for bracketing calls to
the crash reporter with #ifdef MOZ_CRASHREPORTER / #endif preprocessor
directives. You can now freely use the crash reporter methods without
worrying about it being enabled at compiler time or not.
I've also removed all the existing directives and only a few remain
where they are actually needed. JavaScript consumers should be
unaffected save for the following pattern which was used in a few places
to detect if the crash reporter had been compiled:
if ("nsICrashReporter" in Ci &&
Services.appinfo instanceof Ci.nsICrashReporter) {
// Crash reporter is enabled
}
This doesn't work anymore as the nsICrashReporter interface is always
present. You can replace it with:
if (AppConstants.MOZ_CRASHREPORTER) {
// Crash reporter is enabled
}
This touched a lot of places in the code so if anything crash-related
seems wrong please let me know.
Gabriele
[1] Use a dummy crashreporter implementation when building with
--disable-crashreporter
https://bugzilla.mozilla.org/show_bug.cgi?id=1402519
signature.asc
Description: OpenPGP digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Is ReentrantMonitorAutoExit a footgun?
I just came to realize:
ReentrantMonitorAutoEnter lock1(...);
ReentrantMonitorAutoEnter lock2(...);
{
ReentrantMonitorAutoExit unlock1(...);
// This will not release the monitor.
{
ReentrantMonitorAutoExit unlock2(...);
// This will release the monitor.
}
}
Sometimes it is not clear how many ReentrantMonitorAutoExits are required to
effectively release the monitor.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: No more #ifdef MOZ_CRASHREPORTER directives needed when using crash reporter functions
Hi,
I didn't see package-manifest changes in the bug.
I assume this needs to stay in as-is?
Thanks
FRG
+++ snip +++
; [Crash Reporter]
;
#ifdef MOZ_CRASHREPORTER
@RESPATH@/components/CrashService.manifest
@RESPATH@/components/CrashService.js
@RESPATH@/components/toolkit_crashservice.xpt
#ifdef XP_MACOSX
@BINPATH@/crashreporter.app/
#else
@BINPATH@/crashreporter@BIN_SUFFIX@
@RESPATH@/crashreporter.ini
@BINPATH@/minidump-analyzer@BIN_SUFFIX@
#ifdef XP_UNIX
@RESPATH@/Throbber-small.gif
#endif
#endif
@RESPATH@/browser/crashreporter-override.ini
#ifdef MOZ_CRASHREPORTER_INJECTOR
@BINPATH@/breakpadinjector.dll
#endif
#endif
Gabriele Svelto wrote:
[cross-posting to firefox-dev]
Fellow mozillians,
bug 1402519 [1] just landed and it introduces a dummy implementation of
the crash reporter which is built when configuring with
--disable-crash-reporter. This removes the need for bracketing calls to
the crash reporter with #ifdef MOZ_CRASHREPORTER / #endif preprocessor
directives. You can now freely use the crash reporter methods without
worrying about it being enabled at compiler time or not.
I've also removed all the existing directives and only a few remain
where they are actually needed. JavaScript consumers should be
unaffected save for the following pattern which was used in a few places
to detect if the crash reporter had been compiled:
if ("nsICrashReporter" in Ci &&
Services.appinfo instanceof Ci.nsICrashReporter) {
// Crash reporter is enabled
}
This doesn't work anymore as the nsICrashReporter interface is always
present. You can replace it with:
if (AppConstants.MOZ_CRASHREPORTER) {
// Crash reporter is enabled
}
This touched a lot of places in the code so if anything crash-related
seems wrong please let me know.
Gabriele
[1] Use a dummy crashreporter implementation when building with
--disable-crashreporter
https://bugzilla.mozilla.org/show_bug.cgi?id=1402519
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Hiding 'new' statements - Good or Evil?
On Thu, Nov 23, 2017 at 4:00 PM, smaug wrote: > On 11/23/2017 11:54 PM, Botond Ballo wrote: > >> I think it makes sense to hide a 'new' call in a Make* function when >> you're writing an abstraction that handles allocation *and* >> deallocation. >> >> So MakeUnique makes sense, because UniquePtr takes ownership of the >> allocated object, and will deallocate it in the destructor. MakeRefPtr >> would make sense for the same reason. >> > I almost agree with this, but, all these Make* variants hide the > information that they are allocating, > and since allocation is slow, it is usually better to know when allocation > happens. > I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about > the functionality. > This seems like a reasonable argument in isolation, but I think it's more important to mirror the standard C++ mechanisms and C++-14 already defines std::make_unique. As a post-script, given that we now can use C++-14, can we globally replace the MFBT clones of C++-14 mechanisms with the standard ones? -Ekr > > > -Olli > > > >> But in cases where a library facility is not taking ownership of the >> object, and thus the user will need to write an explicit 'delete', it >> makes sense to require that the user also write an explicit 'new', for >> symmetry. >> >> NotNull is a bit of a weird case because it can wrap either a raw >> pointer or a smart pointer, so it doesn't clearly fit into either >> category. Perhaps it would make sense for MakeNotNull to only be >> usable with smart pointers? >> >> Cheers, >> Botond >> >> > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: No more #ifdef MOZ_CRASHREPORTER directives needed when using crash reporter functions
On 24/11/2017 05:18, Frank-Rainer Grahl wrote: > Hi, > > I didn't see package-manifest changes in the bug. > > I assume this needs to stay in as-is? Yes, when we build with --disable-crashreporter we still don't want the various other bits (including the crash reporter UI) to be built and packaged. Only the C++ interface is stubbed with the dummy implementation. Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: No more #ifdef MOZ_CRASHREPORTER directives needed when using crash reporter functions
On Fri, Nov 24, 2017 at 2:55 AM, Gabriele Svelto wrote: > [cross-posting to firefox-dev] > > Fellow mozillians, > bug 1402519 [1] just landed and it introduces a dummy implementation of > the crash reporter which is built when configuring with > --disable-crash-reporter. This removes the need for bracketing calls to > the crash reporter with #ifdef MOZ_CRASHREPORTER / #endif preprocessor > directives. You can now freely use the crash reporter methods without > worrying about it being enabled at compiler time or not. > Thanks for doing this work! These kind of incremental improvements that reduce preprocessing and unify the code base really help over time. Best, Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Is ReentrantMonitorAutoExit a footgun?
Yeah I've always been pretty appalled that this exists. We should remove it
if at all possible.
bholley
On Nov 24, 2017 04:45, wrote:
I just came to realize:
ReentrantMonitorAutoEnter lock1(...);
ReentrantMonitorAutoEnter lock2(...);
{
ReentrantMonitorAutoExit unlock1(...);
// This will not release the monitor.
{
ReentrantMonitorAutoExit unlock2(...);
// This will release the monitor.
}
}
Sometimes it is not clear how many ReentrantMonitorAutoExits are required
to effectively release the monitor.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
