1539498 - Delete copy constructor and operator= for SerializedStructuredCloneBuffer

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Andrew McCreight [:mccr8]]

Copying a SerializedStructuredCloneBuffer can potentially involve allocating a lot of memory. In bug 1539261, I'm making it so that we at least check if the allocation succeeded, but there's nothing that can be done inside an operator= if it fails, so it just crashes. It seems like a bad idea to have this implicit copy operator, so we should probably delete it.

It looks like this is called from the copy constructors for a few IPDL structures that AFAICT aren't actually called. I'm not sure how we can deal with that.

Comment 1

[Andrew McCreight [:mccr8]]

Another possibility would be to just get rid of SerializedStructuredCloneBuffer, which looks like a very thin wrapper around JSStructuredCloneData.

Comment 2

[Simon Giesecke [:sg] [he/him]]

This depends on Bug 1572054, since any IPDL type containing a SerializedStructureCloneBuffer will declare a constructor copying it.

Comment 3

[Nathan Froyd [:froydnj]]

You can annotate SerializedStructuredCloneBuffer in ipdl asmoveonly`, and IPDL should do the right thing with it.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #3)

You can annotate SerializedStructuredCloneBuffer in ipdl asmoveonly`, and IPDL should do the right thing with it.

Oh, thanks! I wasn't aware of that possibility.

Comment 5

[Simon Giesecke [:sg] [he/him]]

Hm, when I add moveonly in https://searchfox.org/mozilla-central/rev/dd1dafd5c9c05640e76af30b58749076e0199704/dom/ipc/DOMTypes.ipdlh#20, from https://searchfox.org/mozilla-central/rev/dd1dafd5c9c05640e76af30b58749076e0199704/dom/ipc/DOMTypes.ipdlh#64 the constructor accepts SerializedStructuredCloneBuffer by rvalue-reference, which is fine. However, also the following code will be generated:

    SerializedStructuredCloneBuffer&
    data()
    {
        return data_;
    }
    SerializedStructuredCloneBuffer&
    data() const
    {
        return data_;
    }

which yields

/home/simon/work/mozilla-unified/obj-x86_64-pc-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/dom/DOMTypes.h:200:16: error: binding reference of type 'mozilla::dom::ClonedMessageData::SerializedStructuredCloneBuffer' (aka 'mozilla::SerializedStructuredCloneBuffer') to value of type 'const mozilla::dom::ClonedMessageData::SerializedStructuredCloneBuffer' (aka 'const mozilla::SerializedStructuredCloneBuffer') drops 'const' qualifier
        return data_;
               ^~~~~

Note the missing const on the return type of the const overload. Do I need to change anything else in DOMTypes.ipdlh or is this a defect in the IPDL generator?

Comment 6

[Nathan Froyd [:froydnj]]

That looks like a bug. If I'm reading the code right, I don't think any of our moveonly types actually get stored in structures (?), so it's definitely possible there are some untried codepaths in there. ISTR a bug about propagating moveonly-ness through structures as well (?).

Comment 7

[Nathan Froyd [:froydnj]]

I think this case should set t.const = True: https://searchfox.org/mozilla-central/source/ipc/ipdl/ipdl/lower.py#587-589 which might solve the problem in comment 5. Propagation of moveonly-ness is still an open question, though.

Comment 8

[Simon Giesecke [:sg] [he/him]]

https://searchfox.org/mozilla-central/rev/dd1dafd5c9c05640e76af30b58749076e0199704/dom/broadcastchannel/BroadcastChannelService.cpp#121 deliberately makes a copy of an IPDL structure containing a SerializedStructuredCloneBuffer. We can probably explicitly clone it at this place.

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1539498 - Make SerializedStructuredCloneBuffer move-only.

Replace all implicit copies by moves, or explicit clone operations.

Depends on D59726

Comment 10

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

So, historically, the way this kind of thing worked is that rvalue references didn't exist yet, so move-only-ish types like Shmem used lvalue references, and the Recv methods would get const references to the containing struct, and this is why the const accessors for those types use const_cast to return non-const lvalue references.

I don't think we propgate moveonly to structs/unions yet (or allow setting it on them manually), but that also isn't necessary until someone wants to move out of a field, and it looks like that's not the case for ClonedMessageData::data.

Comment 11

[Simon Giesecke [:sg] [he/him]]

Well, what's not really that nice is that const_cast is required in the overrides of the generated virtual Alloc* and Recv* functions.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f906da3634a
Make SerializedStructuredCloneBuffer move-only. r=asuth,jld,baku

Comment 13

[Oana Pop-Rus]

Backed out 3 changesets (bug 1539498, bug 1545196) for build bustages failures in DOMTypes.h

/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders/mozilla/dom/DOMTypes.h
/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders/mozilla/dom/ClientIPCTypes.h
/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders/mozilla/dom/ServiceWorkerOpArgs.h

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=287169576&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=0f906da3634aaebe1267d030e31facb82feb2cfc

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287169576&repo=autoland&lineNumber=22644

Backout: https://hg.mozilla.org/integration/autoland/rev/3f81d87f402e1ffe15f31548b4ac03d01a66fec0

Comment 14

[Simon Giesecke [:sg] [he/him]]

(In reply to Oana Pop-Rus from comment #13)

Backed out 3 changesets (bug 1539498, bug 1545196) for build bustages failures in DOMTypes.h

See https://bugzilla.mozilla.org/show_bug.cgi?id=1545196#c19

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d0bcf861a6c
Make SerializedStructuredCloneBuffer move-only. r=asuth,jld,baku

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1d0bcf861a6c