925471 - Dedupe Chromium's base library in security/sandbox with ipc/chromium/src's copy

Status: NEW

(Core :: IPC, defect, P5)

Description

[Brian R. Bondy [:bbondy]]

ipc/chromium/src/base is a partial copy of the chromium base library from 2008.

security/sandbox/base will be a partial copy of the chromium base library from 2013 which has some but not complete overlap.

This bug is to do work to combine them at some point, if we think that is best.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: merge_to_ipc_base.rar WIP

Comment 2

[Brian R. Bondy [:bbondy]]

Attached WIP with most changes needed, probably a couple days of work left though, and only built on Windows so far.

Comment 3

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

I was going to file a bug about this, but apparently there already is one.

Specific things I've run into while working on sandboxing, which I was going to fix case-by-case:

* Logging/assertions.  IPC already has a modified header that maybe does something reasonable, which needs to be extended somewhat to work with newer Chromium code.  The sandbox directory has an unmodified copy that can't be compiled/linked without breaking everything; it should go away.

* Refcounting (as used by the callback/bind stuff).  New-chromium changed the header path and has an incompatible ABI (not that we should be trying to use new headers with old .o files in the first place), but I think the API is backwards-compatible.

* Maybe pickle?  I remember having problems with it, which I worked around by rewriting the code that was using it — and those changes might be upstreamable, in which case I won't have to care about this yet.


Which leaves me with this question: Is there any chance that the earlier work on this bug would be revived in the near future, or should I go ahead with just trying to fix things as I run into them?

Comment 4

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

(In reply to Jed Davis [:jld] from comment #3)
> Which leaves me with this question: Is there any chance that the earlier
> work on this bug would be revived in the near future, or should I go ahead
> with just trying to fix things as I run into them?

On second thought: the more I look at the code involved, the less convinced I am that that kind of ad-hoc hacking is a good idea.  The Windows sandbox's approach of building a separate DLL with the newer chromium/base bits is probably the way to go there.

Comment 5

[(not currently active) Ted Mielczarek]

I've been dealing with getting the ipc/chromium code to build on iOS (bug 682873) and it's a giant headache. Humorously, the upstream Chromium code supports iOS, but our copy is forked and I can't just update to that directly. What we have now is an unmaintainable mess. I think we should either
1) Make the IPC code and the sandboxing code share the same copy of the chromium code. This will probably involve sorting out our local changes to ipc/chromium, but would result in us using a fairly pristine copy of upstream chromium or
2) Rip out ipc/chromium, rewrite the IPC code in terms of native Gecko functionality.

I don't know which is more work, but having an unmaintained fork of the Chromium code on our critical path is not a winning strategy.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think comment 5 is probably bsmedberg's call?

Comment 7

[Benjamin Smedberg]

I am happy with either approach, although I think long-term #2 is better. But I'm not going to do the work ;-)

Comment 8

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

An idea that came up at the sandboxing meeting this morning was to change the C++ namespaces in ipc/chromium/src so that it doesn't conflict with security/sandbox/chromium.  This was more to avoid having to keep the sandboxing code from touching libxul (the specific topic was trying to run unit tests from the imported code, since we already have a framework for using gtest, but this has been an ongoing source of irritation more generally), but it's not completely impossible that it might be useful for incrementally migrating parts of IPC back to upstream chromium.

But it's not quite that easy, because once we wind up with headers from both trees in the same translation unit then we have to deal with macros.  Renaming include guards to not intersect is easy.  Dealing with the logging/assertion macros, which we've done very different things to in the two trees (more or less rewriting the entire header in ipc/chromium/src, vs. leaving the header unchanged but using a shim implementation in security/sandbox/chromium), will be harder, and there are probably other things like that.  Also, even if this works it doesn't fix all of the sandboxing build weirdness; see bug 1101170, for example.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

I'm starting to hit this now. Using simple functions from chromium/base in SandboxBroker code fails because we have multiple copies of basictypes.h and both of them are in the include path of that code.