Closed
Bug 1183485
Opened 9 years ago
Closed 9 years ago
Exempt imported Chromium from MOZ_IMPLICIT enforcement
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox42 fixed)
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
Details
Attachments
(1 file)
1.24 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
security/sandbox/chromium contains code imported from Chromium, and we're trying to modify it as little as possible in order to avoid having it wind up in the same regrettable state as ipc/chromium/src. There really isn't a good reason to try to enforce Mozilla style on it, and especially the check for marking up implicit constructors with MOZ_IMPLICIT. It would be a one-line change to add directories named "chromium" to the exceptions, but that would also include ipc/chromium, which is heavily locally modified and should probably continue to be checked.
Assignee | ||
Comment 1•9 years ago
|
||
There's another color this bikeshed can be painted: just exempt safe_sprintf.h, and accept that this will all happen again when some other file in security/sandbox/chromium offends the clang plugin.
Assignee | ||
Comment 2•9 years ago
|
||
Ehsan: any opinions on the right way to deal with this? (And about who'd be a good reviewer for the patch?) Also, I've locally reproduced the breakage that got bug 1181704 backed out, and the one-line patch to add "safe_sprintf.h" does in fact fix it — but if I'm going to do that instead of whitelisting "chromium" it might make more sense to fold this back into the other bug.
Flags: needinfo?(ehsan)
Comment 3•9 years ago
|
||
I've hit this in the past and in the end I just added MOZ_IMPLICIT to the chromium code, but I'd _much_ rather that I didn't have to do this. Could we have some way of adding ignored directories into the moz.build files? That would seem like a more flexible solution.
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #3) > I've hit this in the past and in the end I just added MOZ_IMPLICIT to the > chromium code, but I'd _much_ rather that I didn't have to do this. Thanks for pointing that out; I'd forgotten about that and/or hadn't thought to grep for it. In that case I'm less in favor of comment #1; I think we should just opt-out all of security/sandbox/chromium and drop those changes. We already have blanket exemptions (either by directory or by C++ namespace) for something like 14 other third-party projects, so that wouldn't be unprecedented. > Could we have some way of adding ignored directories into the moz.build files? > That would seem like a more flexible solution. There'd have to be some way to pass that information in. If the Clang plugin allows adding command-line flags then that would work. But there might be some non-obvious reason why things are set up the way they are.
Comment 5•9 years ago
|
||
The code that whitelists directories for the implicit constructor patch is here: <http://mxr.mozilla.org/mozilla-central/source/build/clang-plugin/clang-plugin.cpp#174>. It creates a reverse iterator over the patch and decides based on the components in the path. Right now it just checks one component of the path, but if you want to exclude security/sandbox/chromium, for example, you should be able to look for the components individually and decide based on more than one. Please let me know if that answers your question. If you would like me to write the patch, I'd be happy to do that. Otherwise, I can also review it for you.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 6•9 years ago
|
||
Try (combined with bug 1181704 plus backing out the existing MOZ_IMPLICIT): https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eba80eccb55 Also tested locally that removing a MOZ_IMPLICIT from ipc/chromium still breaks the build.
Attachment #8635551 -
Flags: review?(ehsan)
Updated•9 years ago
|
Attachment #8635551 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Thanks for the review. checkin-needed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eba80eccb55
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0bf14a4924b3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•6 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•