Closed
Bug 1358223
Opened 8 years ago
Closed 7 years ago
Hard code the lowest allowable content process sandbox at level 1 for Windows/OSX
Categories
(Core :: Security: Process Sandboxing, enhancement)
Core
Security: Process Sandboxing
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jimm, Assigned: Alex_Gaynor)
References
(Blocks 1 open bug)
Details
(Whiteboard: sbwc2, sbmc2, sblc3)
Attachments
(2 files)
We're going to level 3 in our next rollout, so we'll be setting a minimum level of 1 to prevent users from accidentally disabling (or getting tricked into) turning the sandbox off through prefs. We will keep one down level available (the newest) for testing and debugging issues that show up on release channels.
Comment 1•8 years ago
|
||
I'll pick this up at the same time as adding level 3, which I already partly started, before these other file content process regressions emerged.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
![]() |
Reporter | |
Comment 2•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #1) > I'll pick this up at the same time as adding level 3, which I already partly > started, before these other file content process regressions emerged. Thanks!
Assignee: bobowencode → jmathies
Summary: Hard code the content process sandbox at level 1 for Windows/OSX → Hard code the lowest allowable content process sandbox at level 1 for Windows/OSX
Assignee | ||
Updated•8 years ago
|
Assignee: jmathies → bobowencode
Assignee | ||
Comment 3•8 years ago
|
||
(Reasonably certain that :jimm assigning to himself was an accident)
Comment 4•8 years ago
|
||
Let's discuss this before adding the limit for Mac. For Mac: We shipped level=1 in build 52 which blocks most filesystem write access. We are running level=2 on Nightly at the moment which includes no read access to ~/Library and $PROFILE (with some exceptions). We are planning to turn on level=3 in Build 55 which will remove read access to $HOME (at a minimum). Level 2 and above depend on the content file process. Level 2 is acceptable for nightly, but can't be shipped without file content process but it prevents users from opening web files locally in some cases. We expect content file process to be ready to ship in 55. The plan is to skip shipping of level 3 rather than to change the definition of level 2 to avoid confusion (internally) about levels. Of course, that is open for discussion.
Assignee | ||
Comment 5•8 years ago
|
||
> The plan is to skip shipping of level 3
Skip _to_ shipping level 3, right?
Comment 6•8 years ago
|
||
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5) > > The plan is to skip shipping of level 3 > > Skip _to_ shipping level 3, right? Yes, thanks. I meant skip shipping level 2.
Comment 7•8 years ago
|
||
I thought I'd be touching this code, but I won't be, so someone else might get to this before I do. Should be pretty straight forward case of checking when we get the pref security.sandbox.content.level and forcing it to be at least 1. (In reply to Haik Aftandilian [:haik] from comment #6) > (In reply to Alex Gaynor [:Alex_Gaynor] from comment #5) > > > The plan is to skip shipping of level 3 > > > > Skip _to_ shipping level 3, right? > > Yes, thanks. I meant skip shipping level 2. If I understand correctly, it doesn't matter if we ship 2 or 3, as we'll be hard-coding the minimum to 1 either way.
Assignee: bobowencode → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → agaynor
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8861446 -
Flags: review?(bobowencode)
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review136274 Thanks Alex, just a few nits. ::: browser/app/profile/firefox.js (Diff revision 3) > #endif > > #if defined(XP_LINUX) && defined(MOZ_SANDBOX) && defined(MOZ_CONTENT_SANDBOX) > // This pref is introduced as part of bug 742434, the naming is inspired from > // its Windows/Mac counterpart, but on Linux it's an integer which means: > -// 0 -> "no sandbox" This is the linux one, which I don't think we should be changing. ::: dom/ipc/ContentChild.cpp:1416 (Diff revision 3) > } > #elif defined(XP_WIN) > mozilla::SandboxTarget::Instance()->StartSandbox(); > #elif defined(XP_MACOSX) > - sandboxEnabled = StartMacOSContentSandbox(); > + StartMacOSContentSandbox(); > + sandboxEnabled = true; nit: this is already initialised to true ::: ipc/glue/GeckoChildProcessHost.cpp:345 (Diff revision 3) > #if defined(MOZ_CONTENT_SANDBOX) > // We need to get the pref here as the process is launched off main thread. > if (mProcessType == GeckoProcessType_Content) { > mSandboxLevel = Preferences::GetInt("security.sandbox.content.level"); > + if (mSandboxLevel < 1) { > + mSandboxLevel = 1; nit: should only be indented by 2. This occurs in some other places as well. ::: security/sandbox/test/browser_content_sandbox_fs.js:171 (Diff revision 3) > ok(prefExists, "pref security.sandbox.content.level exists"); > if (!prefExists) { > return; > } > > + if (level < 1) { I don't think we should change any of these tests, they would catch it if someone mistakenly changed the pref file. ::: toolkit/xre/nsEmbedFunctions.cpp:316 (Diff revision 3) > AddContentSandboxLevelAnnotation() > { > if (XRE_GetProcessType() == GeckoProcessType_Content) { > int level = Preferences::GetInt("security.sandbox.content.level"); > + if (level < 1) { > + level = 1; All of this makes me think we should have some sort of central settings/configuration class to keep this logic, instead of it being spread throughout the code. Might take a bit to get the design of that right, particularly cross platform. So it should be a follow-up, I've added it for discussion on the roundtable. ::: toolkit/xre/nsXREDirProvider.cpp:771 (Diff revision 3) > IsContentSandboxDisabled() > { > if (!BrowserTabsRemoteAutostart()) { > return false; > } > #if defined(XP_WIN) || defined(XP_MACOSX) Existing issue ... but I'm assuming that we're already in a #if here or this wouldn't compile on Linux. Also, that logic above looks wrong ... if e10s is off (!BrowserTabsRemoteAutostart()) then we should return true, as the sandbox is effectively disabled. I missed that in the original review. :) Anyway, looks like we can just return !BrowserTabsRemoteAutostart() for now. I think it's worth keeping the separate method for now.
Attachment #8861446 -
Flags: review?(bobowencode)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0459f7084b33c06cddb500e31aaf779bcf894f0d&group_state=expanded
Assignee | ||
Updated•8 years ago
|
Attachment #8861446 -
Flags: review?(haftandilian)
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review137410 ::: toolkit/xre/nsAppRunner.cpp:4174 (Diff revision 5) > #if defined(MOZ_CONTENT_SANDBOX) && !defined(MOZ_WIDGET_GONK) > void AddSandboxAnnotations() > { > // Include the sandbox content level, regardless of platform > int level = Preferences::GetInt("security.sandbox.content.level"); > + if (level < 1) { I was just about to r+ this, when I realised that we're going to get a bit of an inconsistency with what we're reporting in telemetry and in about:support. We're not changing toolkit/modules/Troubleshoot.jsm (for about:support). We're also not changing toolkit/components/telemetry/TelemetryEnvironment.jsm, which looks like it can't be done or at least not in a trivial way. We are affecting the Telemetry annotation here and in toolkit/xre/nsEmbedFunctions.cpp. Now in some ways that might be useful, because we'd know the effective sandbox level in telemetry as well as the actual pref, but I think it's worth discussing this at the next meeting before landing. Just to make sure everybody is happy with it and knows about it, because it could be a cause of confusion when debugging future problems.
Assignee | ||
Comment 17•8 years ago
|
||
On IRC yesterday we discussed letting people still set it to 0 on Nightly, so that's also an actionitem for me. Since that'll introduce yet more code at every callsite, I want to introduce a |GetEffectiveSandboxLevel| function. Does anyone have a suggestion on where the right place in the codebase for that is, since it'll be called from all over.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review137410 > I was just about to r+ this, when I realised that we're going to get a bit of an inconsistency with what we're reporting in telemetry and in about:support. > > We're not changing toolkit/modules/Troubleshoot.jsm (for about:support). > We're also not changing toolkit/components/telemetry/TelemetryEnvironment.jsm, which looks like it can't be done or at least not in a trivial way. > > We are affecting the Telemetry annotation here and in toolkit/xre/nsEmbedFunctions.cpp. > > Now in some ways that might be useful, because we'd know the effective sandbox level in telemetry as well as the actual pref, but I think it's worth discussing this at the next meeting before landing. > Just to make sure everybody is happy with it and knows about it, because it could be a cause of confusion when debugging future problems. At a minimum, is there a good place for me to document this so that whoever's debugging next has a good chance at setting the docs?
Comment hidden (mozreview-request) |
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review137852 Looks good. Waiting for discussion on the telemetry issue Bob commented on. ::: dom/ipc/ContentChild.cpp:242 (Diff revision 7) > using namespace mozilla::widget; > > namespace mozilla { > > +int GetEffectiveSandboxLevel() { > + int level = Preferences::GetInt("security.sandbox.content.level"); Low priority corner case question: Could we cache the level here and avoid re-reading the pref and also the possibility of getting into an undefined state after the pref is changed on the fly? There are comments below in ContentParent.cpp (in original code) that imply our Linux sandbox could tolerate security.sandbox.content.level changing during the lifetime of a content process and that's it could be useful. On Mac, we wouldn't want to try to support that. I don't think that would work for Windows. If it's useful on Linux, then I'd vote to leave the code as is. ::: dom/ipc/ContentParent.cpp:2374 (Diff revision 7) > // XXX: Checking the pref here makes it possible to enable/disable sandboxing > // during an active session. Currently the pref is only used for testing > // purpose. If the decision is made to permanently rely on the pref, this > // should be changed so that it is required to restart firefox for the change > // of value to take effect. > - shouldSandbox = (Preferences::GetInt("security.sandbox.content.level") > 0) && > + shouldSandbox = (GetEffectiveSandboxLevel() > 0) && You could add an IsSandboxed() method to hide the numerical level from callers that don't need to know about it.
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review138502 ::: dom/ipc/ContentParent.cpp:2374 (Diff revision 7) > // XXX: Checking the pref here makes it possible to enable/disable sandboxing > // during an active session. Currently the pref is only used for testing > // purpose. If the decision is made to permanently rely on the pref, this > // should be changed so that it is required to restart firefox for the change > // of value to take effect. > - shouldSandbox = (Preferences::GetInt("security.sandbox.content.level") > 0) && > + shouldSandbox = (GetEffectiveSandboxLevel() > 0) && Two of these callers care; I'm not sure it's worth it since one of them is `IsContentSandboxDisabled`, I think it'd just make it more confusing. I did realize that I should put `Content` in the function's name, so I'm going to go do that.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review137410 > At a minimum, is there a good place for me to document this so that whoever's debugging next has a good chance at setting the docs? This now implements the logic we talked about on thursday: telemetry gets the effecitve level, `about:support` gets both.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review141292 ::: dom/ipc/mozISandboxStatus.idl:8 (Diff revision 11) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > + Could use a comment describing the purpose of this interface and why its necessary. ::: dom/ipc/mozISandboxStatus.idl:10 (Diff revision 11) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > + > +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)] > +interface mozISandboxStatus : nsISupports Just a question: is there a rule you followed to use "moz" as the name prefix? ::: dom/ipc/mozISandboxStatus.idl:12 (Diff revision 11) > +#include "nsISupports.idl" > + > +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)] > +interface mozISandboxStatus : nsISupports > +{ > + long getEffectiveContentSandboxLevel(); Should this be a readonly attribute where the C++ implementation is a Set and Get method? I've seen that pattern a lot.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review141292 > Just a question: is there a rule you followed to use "moz" as the name prefix? I _assumed_ (maybe incorrectly!) that the `ns` prefix was mostly for older ones, and `moz` was the new way of doing things. I don't think I saw any docs on naming conventions.
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review141562 Sorry that (as is often the way with software development) this has turned into a more complicated change than I originally thought. :-) (This is doubly true on Gecko.) Thanks for starting to clean this stuff up, still a few more things, I'll clear the review so I know when it's ready again. Mozreview seems to cause a lot of updates, so it's difficult to know otherwise. ::: dom/ipc/ContentChild.h:42 (Diff revision 11) > class RemoteSpellcheckEngineChild; > > +// Return the current sandbox level. This is the > +// "security.sandbox.content.level" preference, but rounded up to the current > +// minimum allowed level. > +int GetEffectiveContentSandboxLevel(); I'm not sure ContentChild is the right place for this. On Windows at least we also read this in the parent, so seems a little odd. Why not just have this code in the new class you have added? It could have a normal header file as well with the same or a static member for C++ access. ::: dom/ipc/ContentChild.cpp:241 (Diff revision 11) > #endif > using namespace mozilla::widget; > > namespace mozilla { > > +int GetEffectiveContentSandboxLevel() { I thought we also agreed to include the effect of the environment variable MOZ_DISABLE_CONTENT_SANDBOX here as well. Maybe that could be a follow-up though. Out of interest, does the env var work for Mac? ::: dom/ipc/mozISandboxStatus.idl:10 (Diff revision 11) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > + > +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)] > +interface mozISandboxStatus : nsISupports I think this should go in security/sandbox. I'm not keen on SandboxStatus. Looking through the code SandboxConfig or SandboxSettings, would seem to fit in more. Also I don't know enough about XPCOM to be confident in reviewing a new implementation. Maybe someone else on the team could check it over. ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979 (Diff revision 11) > if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) { > prefValue = "<user-set>"; > } else { > prefValue = Preferences.get(pref, null); > } > + if (policy.postProcess) { I'm not convinced this is the best solution. Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref. Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch. ::: xpcom/base/nsSystemInfo.cpp (Diff revision 11) > return rv; > } > > NS_IMPL_ISUPPORTS_INHERITED(nsSystemInfo, nsHashPropertyBag, nsIObserver) > #endif // defined(XP_WIN) > - Is this a real change or some mozreview artifact?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review141562 > I thought we also agreed to include the effect of the environment variable MOZ_DISABLE_CONTENT_SANDBOX here as well. Maybe that could be a follow-up though. > > Out of interest, does the env var work for Mac? I thought we'd agreed not to change that behavior -- I didn't realize we wanted it in this patch. Happy to do it, but let's move to a follow up. > I'm not convinced this is the best solution. > > Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref. > > Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch. Agreed that it feels messy. My concern about just moving the value out of the prefs is that it then means telemetry's meaning has changed between before-and-after this patch, which might make analysis harder.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8861446 -
Flags: review?(jmathies)
Attachment #8861446 -
Flags: review?(haftandilian)
Attachment #8861446 -
Flags: review?(bobowencode)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review142254
Attachment #8861446 -
Flags: review?(haftandilian) → review+
![]() |
Reporter | |
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review142610
Attachment #8861446 -
Flags: review?(jmathies) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8861446 -
Flags: review?(bobowencode)
Attachment #8867358 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 41•7 years ago
|
||
:dexter, would love some feedback on the telemetry portion of this, specifically the changes I made adding `postProcess`, so we can get the effective value for the pref, instead of the raw value. Thanks.
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review142946 Thanks for all your work on this. ::: security/sandbox/common/SandboxSettings.cpp:14 (Diff revision 17) > +#include "mozilla/ModuleUtils.h" > +#include "mozilla/Preferences.h" > + > +namespace mozilla { > + > +int GetEffectiveContentSandboxLevel() { Please, file the follow-up on moving the MOZ_DISABLE_CONTENT_SANDBOX env var logic into here as well (if you haven't already :-) ). Having that overriding it to 0, should work I think. It'll be great to have it all in one place and mean we get consistent reporting for when that is used as well. It will also mean we properly take account of it in some areas where we don't currently, that will be good but we'll have to be mindful of potential issues.
Attachment #8861446 -
Flags: review+
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review142950 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:160 (Diff revision 1) > +// Swap out the pref for the effective level to make our telemetry reports more > +// accurate. > +function processSandboxLevel(level) { > + let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"]. > + getService(Ci.mozISandboxSettings); > + return sandboxSettings.effectiveContentSandboxLevel; Can this ever throw? If so, it could disrupt gathering Telemetry for all the prefs :( ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:236 (Diff revision 1) > ["places.history.enabled", {what: RECORD_PREF_VALUE}], > ["privacy.trackingprotection.enabled", {what: RECORD_PREF_VALUE}], > ["privacy.donottrackheader.enabled", {what: RECORD_PREF_VALUE}], > ["security.mixed_content.block_active_content", {what: RECORD_PREF_VALUE}], > ["security.mixed_content.block_display_content", {what: RECORD_PREF_VALUE}], > - ["security.sandbox.content.level", {what: RECORD_PREF_VALUE}], > + ["security.sandbox.content.level", {what: RECORD_PREF_VALUE, postProcess: processSandboxLevel}], Could we please add test coverage for the "postProcess" step in [test_TelemetryEnvironment.js](http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js)? ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979 (Diff revision 1) > if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) { > prefValue = "<user-set>"; > } else { > prefValue = Preferences.get(pref, null); > } > + if (policy.postProcess) { This is only really meaningful for RECORD_PREF_VALUEs, so move it in the previous block.
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review142956 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:237 (Diff revision 1) > ["privacy.trackingprotection.enabled", {what: RECORD_PREF_VALUE}], > ["privacy.donottrackheader.enabled", {what: RECORD_PREF_VALUE}], > ["security.mixed_content.block_active_content", {what: RECORD_PREF_VALUE}], > ["security.mixed_content.block_display_content", {what: RECORD_PREF_VALUE}], > - ["security.sandbox.content.level", {what: RECORD_PREF_VALUE}], > + ["security.sandbox.content.level", {what: RECORD_PREF_VALUE, postProcess: processSandboxLevel}], > ["xpinstall.signatures.required", {what: RECORD_PREF_VALUE}], Can we also document this change [here](https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/toolkit/components/telemetry/docs/data/environment.rst#336)?
Comment 45•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review142950 This looks good overall, thanks!
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review142972 On a second thought, this change is no longer reporting a 'pref' so the preferences section might not be the best place for this change afterall. Can we move it somewhere else, e.g. settings? Sorry for the noise.
Attachment #8867358 -
Flags: review?(alessio.placitelli) → review-
Comment 47•7 years ago
|
||
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #33) > > I'm not convinced this is the best solution. > > > > Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref. > > > > Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch. > > Agreed that it feels messy. My concern about just moving the value out of > the prefs is that it then means telemetry's meaning has changed between > before-and-after this patch, which might make analysis harder. Alex, what's your concern about the analysis?
Flags: needinfo?(agaynor)
Assignee | ||
Comment 48•7 years ago
|
||
My concern is that if someone is tracking some metric, or correlation, involving the sandbox level, the data is no longer consistent between before-and-after this patch. For example, if we were watching "what sandbox level do people" use to see how many people this auto-upgrades, now we have to watch that across two metrics. Does that make sense?
Flags: needinfo?(agaynor)
Assignee | ||
Comment 49•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861446 [details] Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. https://reviewboard.mozilla.org/r/133440/#review142946 > Please, file the follow-up on moving the MOZ_DISABLE_CONTENT_SANDBOX env var logic into here as well (if you haven't already :-) ). > Having that overriding it to 0, should work I think. > > It'll be great to have it all in one place and mean we get consistent reporting for when that is used as well. > > It will also mean we properly take account of it in some areas where we don't currently, that will be good but we'll have to be mindful of potential issues. Bug 1365257
Comment 50•7 years ago
|
||
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #48) > My concern is that if someone is tracking some metric, or correlation, > involving the sandbox level, the data is no longer consistent between > before-and-after this patch. For example, if we were watching "what sandbox > level do people" use to see how many people this auto-upgrades, now we have > to watch that across two metrics. Does that make sense? It does, but the meaning of the "pref" would be changing on the Telemetry side anyway, so one would need to still pay attention to this. This might be less subtle and prone to error by properly moving this information outside the "userPrefs" section.
Assignee | ||
Comment 51•7 years ago
|
||
That's not true -- the concept of an effective value doesn't exist until part 1 of this patch, so both pre-and-post this patch, the value in telemetry is the "currently running value".
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•7 years ago
|
||
I went ahead and implemented your original review comments, on the hope that maybe you found my position compelling :-) If not, reverting is easy enough.
Flags: needinfo?(alessio.placitelli)
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review143100 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979 (Diff revision 2) > if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) { > prefValue = "<user-set>"; > } else { > prefValue = Preferences.get(pref, null); > + if (policy.postProcess) { > + prefValue = policy.postProcess(prefValue); In the light of one of the previous comments, should this have a try/catch?
Assignee | ||
Comment 56•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review143100 > In the light of one of the previous comments, should this have a try/catch? The current `postProcess` function can never raise; I'll defer to :Dexter on what the right style for this code long term is.
Comment 57•7 years ago
|
||
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #51) > That's not true -- the concept of an effective value doesn't exist until > part 1 of this patch, so both pre-and-post this patch, the value in > telemetry is the "currently running value". Good point :-) The issue that remains is that this is not a pref any more, so it seems weird to keep it around like that to make analyses not consider two data points, at least at the beginning. Georg, do you have any opinion on this?
Flags: needinfo?(alessio.placitelli) → needinfo?(gfritzsche)
Comment 58•7 years ago
|
||
This part of the environment lists user prefs, not effective values. I think it's confusing to do this. We have established patterns of adding these things as `settings.foo` properties, you should follow that. If you don't need the original `userPref` entry, just remove it.
Flags: needinfo?(gfritzsche)
Comment 59•7 years ago
|
||
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value Cancelling the review as per comment 58.
Attachment #8867358 -
Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 62•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147940 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1282 (Diff revision 3) > + let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"]. > + getService(Ci.mozISandboxSettings); > + return { > + effectiveContentProcessLevel: sandboxSettings.effectiveContentSandboxLevel > + }; Can you wrap these in a try/catch and return something meaningful if we throw? e.g. setting effectiveContentProcessLevel to null? Let's also document this in environment.rst
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 65•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147968 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1288 (Diff revision 4) > + _getSandboxData() { > + let effectiveContentProcessLevel = null; > + try { > + let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"]. > + getService(Ci.mozISandboxSettings); > + effectiveContentProcessLevel = ( nit: why are the parentheses needed? Can't this simply be effectiveContentProcessLevel = sandboxSettings.effectiveContent; ? ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1292 (Diff revision 4) > + getService(Ci.mozISandboxSettings); > + effectiveContentProcessLevel = ( > + sandboxSettings.effectiveContentSandboxLevel); > + } catch (e) {} > + return { > + effectiveContentProcessLevel, Shouldn't this be (maybe renaming the effectiveContentProcessLevel variable to something like effectiveLevel for clarity :) ): ``` { effectiveContentProcessLevel = effectiveLevel, } ``` ? Otherwise the environment will contain "sandbox: <integer>" instead of "sandbox: { effectiveContentProcessLevel: <integer> }", which is different than what described in the docs
Assignee | ||
Comment 66•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147968 > nit: why are the parentheses needed? Can't this simply be effectiveContentProcessLevel = sandboxSettings.effectiveContent; ? To avoid wrapping beyond 80 columns. If that's not the prefered style, I can make it a long line. > Shouldn't this be (maybe renaming the effectiveContentProcessLevel variable to something like effectiveLevel for clarity :) ): > > ``` > { > effectiveContentProcessLevel = effectiveLevel, > } > ``` > > ? > > Otherwise the environment will contain "sandbox: <integer>" instead of "sandbox: { effectiveContentProcessLevel: <integer> }", which is different than what described in the docs `{name}` is equivilant to `{name: name}` and `eslint` told me to use the short version.
Comment 67•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147982 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1288 (Diff revision 4) > + _getSandboxData() { > + let effectiveContentProcessLevel = null; > + try { > + let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"]. > + getService(Ci.mozISandboxSettings); > + effectiveContentProcessLevel = ( Good point. Given that we do something similar at line 1276, do we need parentheses here? I'm fine with breaking the line without them to keep it within the 80/90 chars.
Comment 68•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147968 > `{name}` is equivilant to `{name: name}` and `eslint` told me to use the short version. Wow, I didn't know that! Thanks, looks good then!
Assignee | ||
Comment 69•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review147982 > Good point. Given that we do something similar at line 1276, do we need parentheses here? > > I'm fine with breaking the line without them to keep it within the 80/90 chars. You're right, no parens needed. I forgot JS is happy with this, too much python :-)
Comment hidden (mozreview-request) |
Comment 71•7 years ago
|
||
mozreview-review |
Comment on attachment 8867358 [details] Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value https://reviewboard.mozilla.org/r/138882/#review148214 This looks good now, thanks for the patience! Please note that we need to update the schema for all the pings using the 'environment' [here](https://github.com/mozilla-services/mozilla-pipeline-schemas/tree/master/schemas/telemetry) too. We don't need to block this bug on this, but can you please file a follow-up bug?
Attachment #8867358 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 72•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 73•7 years ago
|
||
Thanks for the heads up -- made sure all the issues in MozReview were resolved.
Comment 74•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4e283b54baa6 Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. r=bobowen,haik,jimm https://hg.mozilla.org/integration/autoland/rev/852ffb5b89bf Part 2 - In telemetry send the effective sandbox level instead of the raw value r=Dexter
Keywords: checkin-needed
![]() |
||
Comment 75•7 years ago
|
||
Backed out for build bustage on Android at dom/ipc/ContentChild.cpp:21: https://hg.mozilla.org/integration/autoland/rev/8c82d1ad582f2362076dbcb06312ff4606cce8ef https://hg.mozilla.org/integration/autoland/rev/e179fb11fb6a97f60602d46f0ae5fa61f624025c Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=852ffb5b89bf7885b53be6e891ea99717daf421b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=103484419&repo=autoland > /home/worker/workspace/build/src/dom/ipc/ContentChild.cpp:21:37: fatal error: mozilla/SandboxSettings.h: No such file or directory
Flags: needinfo?(agaynor)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(agaynor)
Keywords: checkin-needed
Assignee | ||
Comment 78•7 years ago
|
||
Should be all fixed up now! Confirmed in try that everything looks good on Android once again.
Comment 79•7 years ago
|
||
hg error in cmd: hg pull gecko -r 39b29adcf17531edace6af46ff5c09980ec18e96: pulling from https://reviewboard-hg.mozilla.org/gecko abort: HTTP Error 500: Internal Server Error
Comment 80•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/39941ecd6096 Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. r=bobowen,haik,jimm https://hg.mozilla.org/integration/autoland/rev/533d0f200498 Part 2 - In telemetry send the effective sandbox level instead of the raw value r=Dexter
Keywords: checkin-needed
Comment 81•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39941ecd6096 https://hg.mozilla.org/mozilla-central/rev/533d0f200498
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 82•7 years ago
|
||
> +effectiveContentSandboxLevel = Effective Content Process Sandbox Level Out of l10n interest: Is this about - "the level of the/an effective content process sandbox" i.e. [[Effective Content Process] Sandbox] Level" - "the sandbox level of the/an effective content process" i.e. [[Effective Content Process] Sandbox Level]" - "the effective level of the content process sandbox" i.e. [Effective [Content Process Sandbox] Level]" - or something else? The "ease" of English allows multiple nouns to simply follow up in compound nouns, where a word like "A B C D" usually means "D x C x B x A", where "x" refers to "of/from/for" and D is the actual noun. Longer compounds nouns like these however can be confusing for l10n since miles may vary (i.e. it’s not always like this), so I’m not surporised to see different localizations per locale for this one currently. A l10n note would therefor be welcome, and the existing "Content Process Sandbox Level" string should be clear(er) when this one is. https://transvision.mozfr.org/string/?entity=toolkit/chrome/global/aboutSupport.properties:effectiveContentSandboxLevel&repo=central Also, "effective" may lead up to different interpretations - is this something like "actual/active/working" here? Please don’t reply in a too technical way, not all localizers have developer skills. ;)
Assignee | ||
Comment 83•7 years ago
|
||
It is "the effective level of the content process sandbox". Cheers!
You need to log in
before you can comment on or make changes to this bug.
Description
•