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
This makes the code nicer. In particular, it removes many getter_Copies()
calls. The patch also converts a lot of nsCStrings to nsAutoCString, which will
avoid heap allocation in the common case.
The patch also renames PREF_CopyCharPref() as PREF_GetCStringPref(), because
it's actually getting a string, not a char, and that matches the existing
GetCString() and GetDefaultCString() methods. Correspondingly, it also renames
PREF_SetCharPref() as PREF_SetCStringPref().
The |aPrefName| arguments in nsIPrefBranch.idl remain as |string| because they
almost always involve passing in C string literals, and passing "foo" is much
nicer than passing NS_LITERAL_CSTRING("foo").
It's worth noting that early versions of this patch used |AUTF8String| instead
of |ACString|. But it turns out that libpref stores prefs internally as Latin1.
And |ACString| is compatible with Latin1 but |AUTF8String| isn't, because
non-ASCII Latin1 strings are not valid UTF-8!
MozReview-Commit-ID: D3f7a1Vl1oE
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
This makes the code nicer. In particular, it removes many getter_Copies()
calls. The patch also converts a lot of nsCStrings to nsAutoCString, which will
avoid heap allocation in the common case.
The patch also renames PREF_CopyCharPref() as PREF_GetCStringPref(), because
it's actually getting a string, not a char, and that matches the existing
GetCString() and GetDefaultCString() methods. Correspondingly, it also renames
PREF_SetCharPref() as PREF_SetCStringPref().
The |aPrefName| arguments in nsIPrefBranch.idl remain as |string| because they
almost always involve passing in C string literals, and passing "foo" is much
nicer than passing NS_LITERAL_CSTRING("foo").
It's worth noting that early versions of this patch used |AUTF8String| instead
of |ACString|. But it turns out that libpref stores prefs internally as Latin1.
And |ACString| is compatible with Latin1 but |AUTF8String| isn't, because
non-ASCII Latin1 strings are not valid UTF-8!
These are all easy cases where an nsXPIDLCString local variable is set via
getter_Copies() and then is null checked. The patch uses IsVoid() to replace
the null checks (and get() and EqualsLiteral() calls to replace any implicit
conversions).
Originally the pref is called security.data_uri.inherit_security_context, so the pref is default on.
And when we finish this feature, we turn it off.
To avoid causing confusion in the future, perhaps we should rename it and make the pref default off.
Some protocol handlers don't handle speculative connections too well
(they crash). So, we limit to the most useful protocols. This patch
brought to you by
https://bugzilla.mozilla.org/show_bug.cgi?id=1348278#c21
MozReview-Commit-ID: 8dWGdVtalIS
If the call to NewChannel2 returns NS_ERROR_NOT_IMPLEMENTED or NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED that means the implementation is actually missing, so it is OK to fall back to NewChannel.
If it fails with any other error code, we just return it.
MozReview-Commit-ID: JmgEmPqu6zJ
There's an antipattern where nsLiteralString is used as an unnecessary intermediary in converting from CharT* to CharT*,
e.g. CallAFunctionThatTakesACharPointer(NS_LITERAL_CSTRING("foo").get());
or
NS_NAMED_LITERAL_STRING(foo, "abc");
CallAFunctionThatTakesACharPointer(foo.get());
This patch rewrites the callsites that can be trivially changed to use char*/char16_t*.
I'd somewhat like to remove nsTLiteralString::get() altogether, but in code that's less straightforward than these examples, get() is useful enough to keep.
MozReview-Commit-ID: Kh1rUziVllo
There's an antipattern where nsLiteralString is used as an unnecessary intermediary in converting from CharT* to CharT*,
e.g. CallAFunctionThatTakesACharPointer(NS_LITERAL_CSTRING("foo").get());
or
NS_NAMED_LITERAL_STRING(foo, "abc");
CallAFunctionThatTakesACharPointer(foo.get());
This patch rewrites the callsites that can be trivially changed to use char*/char16_t*.
I'd somewhat like to remove nsTLiteralString::get() altogether, but in code that's less straightforward than these examples, get() is useful enough to keep.
MozReview-Commit-ID: Kh1rUziVllo
* Start/Stop the captive portal service in nsIOService::SetConnectivityInternal
* Set the captive portal state to UNKNOWN when stopping it.
MozReview-Commit-ID: 5dude1F4lNb
The new name makes the sense of the condition much clearer. E.g. compare:
NS_WARN_IF_FALSE(!rv.Failed());
with:
NS_WARNING_ASSERTION(!rv.Failed());
The new name also makes it clearer that it only has effect in debug builds,
because that's standard for assertions.
