This also cleans up some of the unnecessary flags & memebrs which were held
during the changes in bug 1818998. mSelf was unncessary as the runnable
reference will be keeping the instance alive for the same lifetime, and
mSocketThreadShutDown was redundant with !mInitialized.
This change will also fix a potential issue where the socket thread was not
being shut down when switching the browser into an offline state from
`profile-change-net-teardown`, which could've lead to thread leaks when the
browser is returned to an online state. The restore codepath, however, has
been dead for a long time so this is unlikely to be a real issue in practice.
Depends on D178867
Differential Revision: https://phabricator.services.mozilla.com/D178868
While looking at the relevant code, I noticed the unannotated mutex, and added
some annotations to it. The minor changes I had to do to keep the tree
building after the annotations shouldn't be actual threadsafety issues, as
most of it is either dead code, or exclusively called from the only mutating
thread.
Differential Revision: https://phabricator.services.mozilla.com/D178867
HandshakeDone will be called after a handshake is finished and also after the certificate verifications are done.
The code relies on HandshakeDone to signal that the handshake is done. When early-data is not available HandshakeDone is responsible for setting up a Http2 session if needed. There are 2 outcomes when early-data is used:
1) early-data is accepted and transaction continues polling for read,
2) early-data is rejected. In this case, the transaction is restarted as well as polling flags, i.e. the connection will stop polling for read and start polling for write.
Another difference is that a transaction that is started during the early-data period will behave as a normal transaction, i.e. it will write data and continue polling for read to receive response. The special cases during early-data(mWaitingFor0RTTResponse==true) are removed from nsHttpConnection::OnSocketWritable().
EnsureNPNComplete is only responsible for driving handshake and checking the early-data availability. All logic for finishing a handshake (i.e. checking whether early-data is accepted and checking alpn value) has been moved to HandshakeDone.
The patch also extracts FinishNPNSetup that is responsible for the bookkeeping after a handshake is done or fails, e.g. resetting transactions if 0Rtt is used but handshake fails, updating timings and sending telemetry.
HandshakeDone needs to be dispatched so that it is not called inside nss locks. The side effect of this is that nsHttpConnection::OnSocketWritable() may be called in between HandshakeDone being dispatched and executed. Therefore we still need to keep CheckCanWrite0RTTData(). This can be fixed in a follow up patch.
Side cleanups:
Remove mNotTrustedMitmDetected - his was used for ESNI, but it is not used anymore
Differential Revision: https://phabricator.services.mozilla.com/D123824
HandshakeDone will be called after a handshake is finished and also after the certificate verifications are done.
The code relies on HandshakeDone to signal that the handshake is done. When early-data is not available HandshakeDone is responsible for setting up a Http2 session if needed. There are 2 outcomes when early-data is used:
1) early-data is accepted and transaction continues polling for read,
2) early-data is rejected. In this case, the transaction is restarted as well as polling flags, i.e. the connection will stop polling for read and start polling for write.
Another difference is that a transaction that is started during the early-data period will behave as a normal transaction, i.e. it will write data and continue polling for read to receive response. The special cases during early-data(mWaitingFor0RTTResponse==true) are removed from nsHttpConnection::OnSocketWritable().
EnsureNPNComplete is only responsible for driving handshake and checking the early-data availability. All logic for finishing a handshake (i.e. checking whether early-data is accepted and checking alpn value) has been moved to HandshakeDone.
The patch also extracts FinishNPNSetup that is responsible for the bookkeeping after a handshake is done or fails, e.g. resetting transactions if 0Rtt is used but handshake fails, updating timings and sending telemetry.
HandshakeDone needs to be dispatched so that it is not called inside nss locks. The side effect of this is that nsHttpConnection::OnSocketWritable() may be called in between HandshakeDone being dispatched and executed. Therefore we still need to keep CheckCanWrite0RTTData(). This can be fixed in a follow up patch.
Side cleanups:
Remove mNotTrustedMitmDetected - his was used for ESNI, but it is not used anymore
Differential Revision: https://phabricator.services.mozilla.com/D123824
This changeset is the result of adding modernize-use-default-member-init to
tools/clang-tidy/config.yaml then proceeding to run
`./mach static-analysis check netwerk/ --fix`
I then went through the resulting fix and manually updated all of the member
variables which were missed due to them having a non-trivial constructor.
Note that the tool was only run on Linux, so code that only runs on some
platforms may have been missed.
The member variables that are still initialized in the contructor definition
are:
- bitfields (not all currently supported compilers allow default-member-init
- variables that are initialized via a parameter
- variables that use code not visible in the header file
There are a few advantages to landing this change:
- fewer lines of code - now declaration is in the same place as initialization
this also makes it easier to see when looking at the header.
- it makes it harder to miss initializing a member when adding a new contructor
- variables that depend on an include guard look much nicer now
Additionally I removed some unnecessary reinitialization of NetAddr members
(it has a constructor that does that now), and changed nsWifiScannerDBus to
use the thread-safe strtok_r instead of strtok.
Differential Revision: https://phabricator.services.mozilla.com/D116980
This changeset is the result of adding modernize-use-default-member-init to
tools/clang-tidy/config.yaml then proceeding to run
`./mach static-analysis check netwerk/ --fix`
I then went through the resulting fix and manually updated all of the member
variables which were missed due to them having a non-trivial constructor.
Note that the tool was only run on Linux, so code that only runs on some
platforms may have been missed.
The member variables that are still initialized in the contructor definition
are:
- bitfields (not all currently supported compilers allow default-member-init
- variables that are initialized via a parameter
- variables that use code not visible in the header file
There are a few advantages to landing this change:
- fewer lines of code - now declaration is in the same place as initialization
this also makes it easier to see when looking at the header.
- it makes it harder to miss initializing a member when adding a new contructor
- variables that depend on an include guard look much nicer now
Additionally I removed some unnecessary reinitialization of NetAddr members
(it has a constructor that does that now), and changed nsWifiScannerDBus to
use the thread-safe strtok_r instead of strtok.
Differential Revision: https://phabricator.services.mozilla.com/D116980
The STS dispatched the first event before setting mThread member. It was possible that the STS::Run() task got started before mThread was set and causing one of the nsISerialEventTarget methods to read mThread/mInitialised/mShuttingDown value as null or zero and they would have returned an error.
We rewrite and simplify the access to mThread such that it's a lockless operation.
ApplyPortRemapPreference assert that it's running on the taskqueue; however if called before initialization, it would have been run on whatever thread called ApplyPortRemap.
Differential Revision: https://phabricator.services.mozilla.com/D81334
The STS dispatched the first event before setting mThread member. It was possible that the STS::Run() task got started before mThread was set and causing one of the nsISerialEventTarget methods to read mThread/mInitialised/mShuttingDown value as null or zero and they would have returned an error.
We rewrite and simplify the access to mThread such that it's a lockless operation.
ApplyPortRemapPreference assert that it's running on the taskqueue; however if called before initialization, it would have been run on whatever thread called ApplyPortRemap.
Differential Revision: https://phabricator.services.mozilla.com/D81334
