1521120 - LoadInfo, Principal and CSP deserialization code takes `const` arguments making it difficult to avoid copies when deserializing

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[:Gijs (he/him)]

It'd be better if we had more efficient implementations, but esp. the LoadInfoArgsToLoadInfo implementation is not trivial. :froydnj also said that improving this will likely take ipdl changes.

Comment 1

[Nathan Froyd [:froydnj]]

The reason this requires IPDL changes is twofold:

• our IPC actor Recv* methods take IPDL-defined structs by const&, e.g.:

https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PNeckoParent.cpp#2116

which basically requires propagating const& all over the place, even though those references should really be &&, since the objects are newly constructed on the stack and won't be used after our actor implementation consumes them.

• one we do the first thing, we should probably consider how to generate move constructors for IPDL structs. People may not use them right away, but they will probably want to eventually... (this part may not actually block the work here)

Once that's all done, then we can go about rewriting PrincipalInfoToPrincipal, LoadInfoArgsToLoadInfo, etc. with versions that take && and/or turn them into giant templated monstrosities so we don't have to write both the const& and && versions out by hand.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

We already have move constructors for IPDL structs, and devirtualization (bug 1512990) should allow ergonomic Recv-by-move, because the autogenerated code can just std::move everything and the argument types can be anything initializable by T&&. So maybe this bug should depend on that instead of bug 1479930.

Send-by-move is harder and probably doesn't matter for this, since it's going to be copied during serialization anyway.

Comment 3

[Nathan Froyd [:froydnj]]

I looked at this some this morning. The plan was to:

• Make everything that takes const OptionalLoadInfoArgs& taken OptionalLoadInfoArgs&& instead.
• Make LoadInfoArgsToLoadInfo pass T&& into its callees, where appropriate.
• Modify callees as necessary to consume T&& rather than const T&, particularly for PrincipalInfo args. PrincipalInfo is an interesting case, because we do need to keep the const PrincipalInfo& conversion around for various reasons.

I ran into some actor implementations that need to be devirtualized (PWebrtcProxyChannel, particularly), and a few other issues. I'll try to fix those next week.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

As mentioned before, Devirtualization should make this much easier to do :-), and that's landed now.