Open Bug 1023619 Opened 6 years ago Updated 2 years ago

nr_stun_client_process_response needs some refactoring

Categories

(Core :: WebRTC: Networking, defect, P5, minor)

defect

Tracking

()

Blocking Flags:
backlog tech-debt

People

(Reporter: bwc, Unassigned)

Details

There are a few things that bug me about nr_stun_client_process_response:

- One large function that handles two distinct duties; determining whether a STUN response's transaction id matches that of the stun context, and performing the processing of that response if so. Writing an abort: section that makes sense for both of these cases is a bit fiddly, particularly wrt logging errors.

- This function overwrites ctx->response even when the transaction id does not match, which is probably asking for confusion.

- This function decodes the STUN response, meaning that we decode the same response multiple times while searching for a matching context. This is a bit silly, although probably not that harmful.

- It is unclear to me whether we even need a |response| field in nr_stun_client_ctx; the only places that use it outside of this function are only interested in the error code, which is recorded in another field already.
backlog: --- → tech-debt
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.