Bugzilla

So, looking at this, right now nICEr keeps the set of ICE servers in the main ice context. We probably need to copy this set of ICE servers when creating each nr_ice_media_stream, so each stream has its own copy. This will allow the use of different sets of ICE servers for each stream so we don't run into dangling pointer problems when changing the set. This will also permit gathering to continue (and ICE to connect) on old streams, if for some reason JS waits after calling setConfiguration, which will mesh better with the way we handle ICE restart and rollback. I am not sure what the spec says to do if the ICE servers are changed with setConfiguration, gathering is started with sLD, and then that sLD is rolled back, but it seems reasonable that the code would continue trying to use the old servers.

We will also need to break out the ICE server validation code into a separate function on MediaTransportHandler; right now that is built into CreateIceCtx, but that is only ever called once. Additionally, we need to do this validation synchronously, because setConfiguration is synchronous. We _probably_ would not want to actually set anything on the NrIceCtx until we get to the StartIceGathering step, for simplicity's sake.

On the subject of ICE transport policy, we probably need to handle this identically to the ICE servers; each nr_ice_media_stream would need to have its own flags field. It would be fine if we copied from the flags field in nr_ice_ctx, probably. This would have the same desirable properties wrt ICE restart and rollback as above.

Lastly, while we have not yet implemented ICE candidate pool logic, the size of this pool can be tweaked with setConfiguration, so we need to think a little about what implementing that would look like. Presumably, with ICE candidate pooling we would either need to pre-create nr_ice_media_streams for later use, or have some other object that serves as a candidate set inside nICEr, which would then have its own copy of the necessary configuration. Also, there is a wrinkle here; ICE pool gathering starts _immediately_ on setConfiguration. This implies that changing either the ICE servers or policy should result in the old pool being discarded immediately as well, even if the pool size has not changed. The spec explicitly mentions this for configuration.iceServers, but _not_ for configuration.iceTransportPolicy (although it _does_ say that "no action will be taken until the next gathering phase", which could be interpreted to apply to pooled ICE candidates). This may require some spec clarification.

On the subject of testing, there is a little bit of coverage in wpt, but it is extremely limited because wpt does not (and can not) test the actual use of ICE servers, or ICE transport policy, because wpt does not have the ability to use STUN/TURN servers. We will need to write some mochitests for this, in addition to enabling some of the wpt suite.

So, looking at this, right now nICEr keeps the set of ICE servers in the main ice context. We probably need to copy this set of ICE servers when creating each nr_ice_media_stream, so each stream has its own copy. This will allow the use of different sets of ICE servers for each stream so we don't run into dangling pointer problems when changing the set. This will also permit gathering to continue (and ICE to connect) on old streams, if for some reason JS waits after calling setConfiguration, which will mesh better with the way we handle ICE restart and rollback. I am not sure what the spec says to do if the ICE servers are changed with setConfiguration, gathering is started with sLD, and then that sLD is rolled back, but it seems reasonable that the code would continue trying to use the old servers until an ICE restart is actually carried out.

We will also need to break out the ICE server validation code into a separate function on MediaTransportHandler; right now that is built into CreateIceCtx, but that is only ever called once. Additionally, we need to do this validation synchronously, because setConfiguration is synchronous. We _probably_ would not want to actually set anything on the NrIceCtx until we get to the StartIceGathering step, for simplicity's sake.

On the subject of ICE transport policy, we probably need to handle this identically to the ICE servers; each nr_ice_media_stream would need to have its own flags field. It would be fine if we copied from the flags field in nr_ice_ctx, probably. This would have the same desirable properties wrt ICE restart and rollback as above.

Lastly, while we have not yet implemented ICE candidate pool logic, the size of this pool can be tweaked with setConfiguration, so we need to think a little about what implementing that would look like. Presumably, with ICE candidate pooling we would either need to pre-create nr_ice_media_streams for later use, or have some other object that serves as a candidate set inside nICEr, which would then have its own copy of the necessary configuration. Also, there is a wrinkle here; ICE pool gathering starts _immediately_ on setConfiguration. This implies that changing either the ICE servers or policy should result in the old pool being discarded immediately as well, even if the pool size has not changed. The spec explicitly mentions this for configuration.iceServers, but _not_ for configuration.iceTransportPolicy (although it _does_ say that "no action will be taken until the next gathering phase", which could be interpreted to apply to pooled ICE candidates). This may require some spec clarification.

On the subject of testing, there is a little bit of coverage in wpt, but it is extremely limited because wpt does not (and can not) test the actual use of ICE servers, or ICE transport policy, because wpt does not have the ability to use STUN/TURN servers. We will need to write some mochitests for this, in addition to enabling some of the wpt suite.