1035275 - Imported Chromium code under security/sandbox that is not being compiled.

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Bob Owen (:bobowen)]

Attachment: Remove unused base Chromium code

While working on bug 1018966, I noticed that there are some cc files that have been imported from Chromium that are not currently being compiled.

For the sandboxing code this seems to be a deliberate choice to import all files from a directory.

However, given that we are not importing all of the Chromium base code, it seems sensible to remove any code that we are not using.
aklotz concurred in Bug 1018966 Comment 23.

After checking to see which cc files were not being compiled, I've also gone through some iterations of grepping for and removing unused header files.

Here's a patch to remove the files that don't appear to be needed.

The only things I didn't include were path_service.h (because I need it for bug 1018966) and sdkdecls.h (as this is our own file and the comment at the top of it indicates that we do need it).

Here's a try push on top of the patches of bug 1018966:
https://tbpl.mozilla.org/?tree=Try&rev=07ed1804e9bb

Obviously things are failing, but these seem pretty consistent with the results without this patch.

Comment 1

[Bob Owen (:bobowen)]

Here's another try run without the patch:
https://tbpl.mozilla.org/?tree=Try&rev=4f9691dea47c

The results are pretty much the same, the only differences will pass if you try them enough times.

bbondy: would you be OK for a review on this?

Comment 2

[Brian R. Bondy [:bbondy]]

sure thing, just mark me for review if it's ready.

Comment 3

[Bob Owen (:bobowen)]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> sure thing, just mark me for review if it's ready.

Thanks Brian, just realised today as I was looking at someone's build error on IRC that I might need to at least check this compiles for some other platforms.

I've kicked off a try run:
https://tbpl.mozilla.org/?tree=Try&rev=499820e1b2dd

We have people round, so I'll probably flag for review tomorrow, if it works OK.

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8451720 [details] [diff] [review]
Remove unused base Chromium code

(In reply to Bob Owen (:bobowen) from comment #3)

> I've kicked off a try run:
> https://tbpl.mozilla.org/?tree=Try&rev=499820e1b2dd

This only built successfully on B2G, the Linux builds are partly failing for some warnings being treated as errors and partly because of the problem jvoisin found in bug 1035786.

Here's a try push with jvoisin's patch and another to fix the member variable initialisation, looks like there are still others:
https://tbpl.mozilla.org/?tree=Try&rev=1e4f49c1fab3

Anyway, I don't think any of these problems are down to removing these files and I've built locally on Linux, which doesn't flag these warnings as errors, so I think we are safe to remove these files.

Comment 5

[Brian R. Bondy [:bbondy]]

Sorry for the delay on reviewing, I'm currently traveling. I'll try to get it in during the weekend, but if not on Monday.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 8451720 [details] [diff] [review]
Remove unused base Chromium code

Review of attachment 8451720 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for cleaning this up!

Comment 7

[Bob Owen (:bobowen)]

Thanks Brian.

Try push is academic for this as none of it compiles for mozilla-central at the moment, but here's a try push with --enable-context-sandbox.
https://tbpl.mozilla.org/?tree=Try&rev=e9ca8951f551

Build failures for Linux 32 bit are down to Bug 1037215 and test failures are known issues test failures with the content sandbox.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b284d6ecc560

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b284d6ecc560