1183485 - Exempt imported Chromium from MOZ_IMPLICIT enforcement

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

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

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.

Comment 1

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

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.

Comment 2

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

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.

Comment 3

[Bob Owen (:bobowen) (PTO until 17th Oct)]

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.

Comment 4

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

(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

[(no longer active)]

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.

Comment 6

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

Attachment: Patch: ignore .../sandbox/chromium

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.

Comment 7

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

Thanks for the review.

checkin-needed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eba80eccb55

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf14a4924b3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bf14a4924b3