Closed Bug 1438446 Opened 2 years ago Closed 11 months ago

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

Categories

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

Other Branch
defect

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

Attached file 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
Thanks for filing, I encountered this last week also, but got distracted and forgot about it.
Product: Core → Firefox Build System
> 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?
Blocks: 1486554
Flags: needinfo?(davidp99)
will do
Assignee: nobody → davidp99
Flags: needinfo?(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)`.
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...
Attached patch PatchSplinter Review
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)
Attachment #9014112 - Flags: review?(dmajor) → review?(jld)
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 kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ad4fb54981
Use the nsTArray copy constructor for the non-memmovable IpdlTupleElement on Windows. r=jld
https://hg.mozilla.org/mozilla-central/rev/52ad4fb54981
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: davidp99 → kats
You need to log in before you can comment on or make changes to this bug.