Because of the storage::Service's connection list, it's possible for the
refcount for a non-main-thread Connection to experience transient increases
and decreases at any time, dooming logic in Release() that assumes the
refcount isn't changing.
This patch adopts use of an Atomic<bool> so that we execute cleanup logic
exactly once when the refcount falls to 1 at some point. Care is taken to
ensure that the failsafe Close() occurs on the correct thread.
SpinningSynchronousClose() is still dangerous and can still potentially
nest deeply on the stack. If we see instances of that in the future, we
may want to adopt use of PushEventQueue so that we can avoid re-entrancy
in our event loop spinning.
MozReview-Commit-ID: A835HBec50H
--HG--
extra : rebase_source : af2f63e8f050b7a0275e39f73e59133958e29f19
This is a follow-up to bug 1409249. There are a lot of places where our
factory singleton constructors either don't correctly handle their returned
references being released by the component manager, or do handle it, but in
ways that are not obvious.
This patch handles a few places where we can sometimes wind up with dangling
singleton pointers, adds some explanatory comments and sanity check
assertions, and replaces some uses of manual refcounting with StaticRefPtr and
ClearOnShutdown.
There are still some places where we may wind up with odd behavior if the
first QI for a getService call fails. In those cases, we wind up destroying
the first instance of a service that we create, and re-creating a new one
later.
MozReview-Commit-ID: ANYndvd7aZx
--HG--
extra : rebase_source : acfb0611a028fef6b9387eb5d1d9e285782fbc7c
Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton
constructors to return already-addrefed raw pointers, and while it accepts
constructors that return already_AddRefed, most existing don't do so.
Meanwhile, the convention elsewhere is that a raw pointer return value is
owned by the callee, and that the caller needs to addref it if it wants to
keep its own reference to it.
The difference in convention makes it easy to leak (I've definitely caused
more than one shutdown leak this way), so it would be better if we required
the singleton getters to return an explicit already_AddRefed, which would
behave the same for all callers.
This also cleans up several singleton constructors that left a dangling
pointer to their singletons when their initialization methods failed, when
they released their references without clearing their global raw pointers.
MozReview-Commit-ID: 9peyG4pRYcr
--HG--
extra : rebase_source : 2f5bd89c17cb554541be38444672a827c1392f3f
NS_ProxyRelease will immediately release the provided reference if already on
the right thread unless aAlwaysRelease=true (the default is false). We need to
set it to be true to avoid Close() and its potential to, out of desperation
when faced with misuse, spin a nested event loop.
--HG--
extra : rebase_source : 91c70a6fa081fdf20a8dfabf5d69bccbef1ba33c
The observer references were the only thing guranteed to keep the Service
alive, leading to potential use-after-free during the iteration loop to
make sure all the connections were closed. (Ironically, if they were
fully closed and their instances destroyed, that's when bad things would
happen.)
--HG--
extra : rebase_source : 6c8d6f9e0b75751b10166d7e2c63d5a3cb27d28e
Per bug: it can take around 200ms to shrink memory for
Places.sqlite. This can end up janking the browser if we hit a
memory-pressure. This patch simply removes the #if DEBUG guards
around the mAsyncExecutionThreadIsAlive boolean which is already
being updated and exposes it so that we can shrink memory off
the main thread when possible.
MozReview-Commit-ID: LoDGKrOXs8u
--HG--
extra : rebase_source : adf03f187e7297835566f3bac92a8be8a6147131
Avoids a thread-safety race condition on shutdown where we could try to finalize a statement twice.
Allows the async thread to be referenced until ShutdownAsyncThread, so async finalizers can make use of it.
Removes the no more useful mAsyncExecutionThreadIsAlive.
Nullifies the sqlite3_mutex pointer when the connection is closed, since it would be a dangling pointer.
Use a ScopeExit to ensure the connection and the mutex pointers are always nullified on failure.
Makes asyncClose bailout early if a Close method was already invoked before.
Makes AsyncInitDatabase not use AsyncClose to just shutdown the async thread.
Fixes various unfinalized statements in consumers code.
Makes mConnectionClosed better follow mDBConn status.
Replaces some mutex locking isClosed() calls with lockless isConnectionReadyOnThisThread.
MozReview-Commit-ID: 6sftFehsQTt
--HG--
extra : rebase_source : 2bf3088b500376e58e62e8f078d9950588adc649
This function is arguably nicer than calling NS_ProcessNextEvent
manually, is slightly more efficient, and will enable better auditing
for NS_ProcessNextEvent when we do Quantum DOM scheduling changes.
This patch removes checking of all the callback calls in memory reporter
CollectReport() functions, because it's not useful.
The patch also does some associated clean-up.
- Replaces some uses of nsIMemoryReporterCallback with the preferred
nsIHandleReportCallback typedef.
- Replaces aCallback/aCb/aClosure with aHandleRepor/aData for CollectReports()
parameter names, for consistency.
- Adds MOZ_MUST_USE/[must_use] in a few places in nsIMemoryReporter.idl.
- Uses the MOZ_COLLECT_REPORT macro in all suitable places.
Overall the patch reduces code size by ~300 lines and reduces the size of
libxul by about 37 KiB on my Linux64 builds.
--HG--
extra : rebase_source : e94323614bd10463a0c5134a7276238a7ca1cf23
This patch makes most Run() declarations in subclasses of nsIRunnable have the
same form: |NS_IMETHOD Run() override|.
As a result of these changes, I had to add |override| to a couple of other
functions to satisfy clang's -Winconsistent-missing-override warning.
--HG--
extra : rebase_source : 815d0018b0b13329bb5698c410f500dddcc3ee12
The bulk of this commit was generated with a script, executed at the top
level of a typical source code checkout. The only non-machine-generated
part was modifying MFBT's moz.build to reflect the new naming.
CLOSED TREE makes big refactorings like this a piece of cake.
# The main substitution.
find . -name '*.cpp' -o -name '*.cc' -o -name '*.h' -o -name '*.mm' -o -name '*.idl'| \
xargs perl -p -i -e '
s/nsRefPtr\.h/RefPtr\.h/g; # handle includes
s/nsRefPtr ?</RefPtr</g; # handle declarations and variables
'
# Handle a special friend declaration in gfx/layers/AtomicRefCountedWithFinalize.h.
perl -p -i -e 's/::nsRefPtr;/::RefPtr;/' gfx/layers/AtomicRefCountedWithFinalize.h
# Handle nsRefPtr.h itself, a couple places that define constructors
# from nsRefPtr, and code generators specially. We do this here, rather
# than indiscriminantly s/nsRefPtr/RefPtr/, because that would rename
# things like nsRefPtrHashtable.
perl -p -i -e 's/nsRefPtr/RefPtr/g' \
mfbt/nsRefPtr.h \
xpcom/glue/nsCOMPtr.h \
xpcom/base/OwningNonNull.h \
ipc/ipdl/ipdl/lower.py \
ipc/ipdl/ipdl/builtin.py \
dom/bindings/Codegen.py \
python/lldbutils/lldbutils/utils.py
# In our indiscriminate substitution above, we renamed
# nsRefPtrGetterAddRefs, the class behind getter_AddRefs. Fix that up.
find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \
xargs perl -p -i -e 's/nsRefPtrGetterAddRefs/RefPtrGetterAddRefs/g'
if [ -d .git ]; then
git mv mfbt/nsRefPtr.h mfbt/RefPtr.h
else
hg mv mfbt/nsRefPtr.h mfbt/RefPtr.h
fi
--HG--
rename : mfbt/nsRefPtr.h => mfbt/RefPtr.h
The bulk of this commit was generated by running:
run-clang-tidy.py \
-checks='-*,llvm-namespace-comment' \
-header-filter=^/.../mozilla-central/.* \
-fix
Andrew Sutherland
Kris Maglione
Gabriele Svelto
shindli
Andrew McCreight
Bob Owen
Sebastian Hengst
Kai Engert
Andrea Marchesini
Olli Pettay
Bill McCloskey
Carsten "Tomcat" Book
Wes Kocher
Doug Thayer
Bevis Tseng
Marco Bonardo
Nathan Froyd
Zibi Braniecki
Gijs Kruitbosch
Phil Ringnalda
Nicholas Nethercote
Akshay CV
Kyle Huey
Aidin Gharibnavaz
Birunthan Mohanathas