Bug 1751818 Comment 5 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

TLDR: this was an explicit design choice but we should probably move away from it. The fix is simple, but will take me a few days. We can either add an exception in the short term or I can submit a PR fixing the source of the problem. 

@tsmith: Let me know how we should proceed.

Capturing some of the design rationale below

--------------

RLBox has a few different types, two of them being `tainted_opaque` and `tainted`. The goal of this design involving these two separate types was:
1. First it reduces compilation times: the tainted structures do a lot of compile time checks and we try to avoid using `tainted` in headers in favor of `tainted_opaque`
2. This previously allowed us use C++17 features when Firefox was still on C++11/14 (this was about 2 and half years ago, so is not something of concern anymore)

While there are suitable conversion operators between `tainted` and `tainted_opaque`, there is one part of the code that assumes these two types have the same layout and this is in indirect calls as seen above. These was by design and in theory should not cause any issues. However given that this is causing UBSan warnings, i think the best path is to move away from this and stick to using `tainted` everywhere.
TLDR: this was an explicit design choice but we should probably move away from it. The fix is simple, but will take me a few days. We can either add an exception in the short term or I can submit a PR fixing the source of the problem. 

@tsmith: Let me know how we should proceed.

Capturing some of the design rationale below

--------------

RLBox has a few different types, two of them being `tainted_opaque` and `tainted`. The goal of this design involving these two separate types was:
1. First it reduces compilation times: the tainted structures do a lot of compile time checks and we try to avoid using `tainted` in headers in favor of `tainted_opaque`
2. This previously allowed us use C++17 features when Firefox was still on C++11/14 (this was about 2 and half years ago, so is not something of concern anymore)

While there are suitable conversion operators between `tainted` and `tainted_opaque`, there is one part of the code that assumes these two types have the same layout and this is in indirect calls as seen above. These was by design and in theory should not cause any issues. However given that this is causing UBSan warnings, i think the best path is to move away from this and stick to using `tainted` everywhere.  A more minimal fix for the issue would be to remove the use of `tainted_opaque` in callbacks alone

Back to Bug 1751818 Comment 5