1438446 - ipdlTuple.h tries to use non-memmovable wstring in nsTArray

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Build output

Hot off the heels of getting a clang build working on Windows, I tried to --enable-clang-plugin and build with that. It proceeded but eventually failed with what looks like a legitimate error.

Build log is attached, but basically the nsTArray at [1] contains a bunch of IpdlTupleElement [2] instances, which are variants that might contain a OpenFileNameIPC [3] struct, which contains a std::wstring [4] (actually it contains a few wstring fields). The clang plugin complains because nsTArray has MOZ_NEEDS_MEMMOVABLE_TYPE [5] but wstring is not guaranteed to be memmove-able.

This seems like a legitimate complaint, which makes me wonder why our static analysis builds in automation didn't pick this up.

[1] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/IpdlTuple.h#82
[2] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/IpdlTuple.h#68
[3] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/FunctionBrokerIPCUtils.h#138
[4] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/FunctionBrokerIPCUtils.h#121
[5] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/xpcom/ds/nsTArray.h#690

Comment 1

[(Away)]

Thanks for filing, I encountered this last week also, but got distracted and forgot about it.

Comment 2

[(Away)]

> This seems like a legitimate complaint, which makes me wonder why our static
> analysis builds in automation didn't pick this up.

Clang versions prior to 7 somehow didn't catch this, probably for the same reason as bug 1492743.

This will prevent us from moving Windows static analysis into our primary build tasks. :handyman, would you be able to look into this?

Comment 3

[David Parks [:handyman]]

will do

Comment 4

[(Away)]

Oh, on closer look this may be easier than I thought...

> The clang plugin complains because nsTArray has MOZ_NEEDS_MEMMOVABLE_TYPE 

That's just the default implementation of nsTArray_CopyChooser. If you're not memmove-able, you can override it. This might be as simple as `DECLARE_USE_COPY_CONSTRUCTORS(IpdlTupleElement)`.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

It turned out to be a little more complicated since IpdlTupleElement is private inside IpdlTuple (i.e. can't forward-declare it), and the DECLARE_USE_COPY_CONSTRUCTORS needs to be outside the mozilla namespace, and yet the DECLARE_USE_COPY_CONSTRUCTORS needs to happen before the array is defined. I had to move the IpdlTupleElement and related things outside IpdlTuple to get it working. I'll put up a try push shortly once I confirm my local build doesn't have any other errors.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a410c6df917e0e23f48614796d2aafe909e119

This error seems fixed, but there's some other error now. My local build is still going...

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

At any rate, this patch should be ok to go in. Suggestions on how to make it less ugly are welcome.

Comment 8

[David Parks [:handyman]]

Thanks.  FWIW, I prefer this namespace setup anyway.

Comment 9

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

Comment on attachment 9014112 [details] [diff] [review]
Patch

Review of attachment 9014112 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.

The only other thing I could think of is to use nsString instead of std::wstring in OpenFileNameIPC (see also bug 1447674), but that might be more trouble than it's worth, especially with wchar_t vs. char16_t.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ad4fb54981
Use the nsTArray copy constructor for the non-memmovable IpdlTupleElement on Windows. r=jld

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/52ad4fb54981