Closed Bug 1438446 Opened 2 years ago Closed 11 months ago
Tuple .h tries to use non-memmovable wstring in ns TArray
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  contains a bunch of IpdlTupleElement  instances, which are variants that might contain a OpenFileNameIPC  struct, which contains a std::wstring  (actually it contains a few wstring fields). The clang plugin complains because nsTArray has MOZ_NEEDS_MEMMOVABLE_TYPE  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.  https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/IpdlTuple.h#82  https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/IpdlTuple.h#68  https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/FunctionBrokerIPCUtils.h#138  https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/dom/plugins/ipc/FunctionBrokerIPCUtils.h#121  https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/xpcom/ds/nsTArray.h#690
Thanks for filing, I encountered this last week also, but got distracted and forgot about it.
> 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?
Assignee: nobody → davidp99
Priority: -- → P1
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)`.
11 months ago
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.
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...
At any rate, this patch should be ok to go in. Suggestions on how to make it less ugly are welcome.
Attachment #9014112 - Flags: review?(dmajor)
Thanks. FWIW, I prefer this namespace setup anyway.
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.
Attachment #9014112 - Flags: review?(jld) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52ad4fb54981 Use the nsTArray copy constructor for the non-memmovable IpdlTupleElement on Windows. r=jld
11 months ago
Assignee: davidp99 → kats
You need to log in before you can comment on or make changes to this bug.