Closed Bug 1259601 Opened 9 years ago Closed 8 years ago

Add sandbox status to about:support (Windows)

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox48 --- wontfix
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: jimm, Assigned: bobowen)

References

Details

(Whiteboard: sbwc1)

Attachments

(2 files)

AFAICT we don't have anything in about:support about sandbox state on Windows.
Whiteboard: sb? → sbwc1
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
I had to make some of the linux test schema to not be required to use the same structure. I could make the structure more complicated and to be OS specific, but I'm not sure it is worth it. MozReview-Commit-ID: HFRiEbkEztp
Attachment #8780834 - Flags: review?(jld)
Attachment #8780834 - Flags: review?(dtownsend)
Attachment #8780834 - Flags: review?(dtownsend) → review+
Comment on attachment 8780834 [details] [diff] [review] Add content process sandbox level to about:support sandboxing information Review of attachment 8780834 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/tests/browser/browser_Troubleshoot.js @@ +438,5 @@ > required: false, > type: "object", > properties: { > hasSeccompBPF: { > + required: false, Not sure if it's worth the effort, but would it be possible to set these based on AppConstants.platform?
Attachment #8780834 - Flags: review?(jld) → review+
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #5) > > hasSeccompBPF: { > > + required: false, > > Not sure if it's worth the effort, but would it be possible to set these > based on AppConstants.platform? Hadn't thought of doing it that way, thanks jld. Seems to work locally (and this test was failing for me locally before, I've done a pull since then, but it must be a good sign :-) ). Try push with that change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=263aae5917dbc922c0fcaddf0e5834cbcdd1949a
(In reply to Bob Owen (:bobowen) (less responsive until 26th) from comment #6) > (In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #5) > > > > hasSeccompBPF: { > > > + required: false, > > > > Not sure if it's worth the effort, but would it be possible to set these > > based on AppConstants.platform? > > Hadn't thought of doing it that way, thanks jld. Just realised while packing that I should use a similar trick for the one I've just added: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fbf004eed0489d9cfb3c9cd7b575a1a4a76f0fe
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/03d58bbdfc34 Add content process sandbox level to about:support sandboxing information. r=jld, r=mossop
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Nightly 48.0a1(2016-03-24) on Windows 10, 64 bit. The Bug's fix is now verified on latest Nightly 51.0a1 Build ID 20160820030224 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 [bugday-20160817]
I don't think we want to uplift that.
Comment on attachment 8780834 [details] [diff] [review] Add content process sandbox level to about:support sandboxing information Note to sheriff: patch that landed is slightly different from one attached to bug. Approval Request Comment [Feature/regressing bug #]: Give content sandbox information in about:support - currently set to roll out in Fx50 on Windows. [User impact if declined]: Might make diagnosing sandboxing issues more difficult. [Describe test coverage new/current, TreeHerder]: Automated about:support test and manual testing. [Risks and why]: Low - on Nightly for a 2+ weeks, fairly straight forward change to about:support page. [String/UUID change made/needed]: Adds new string property to toolkit/locales/en-US/chrome/global/aboutSupport.properties.
Attachment #8780834 - Flags: approval-mozilla-aurora?
Comment on attachment 8780834 [details] [diff] [review] Add content process sandbox level to about:support sandboxing information Fix was verified on Nightly, Aurora50+
Attachment #8780834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Bob Owen (:bobowen) (less responsive until 26th) from comment #12) > [String/UUID change made/needed]: > Adds new string property to > toolkit/locales/en-US/chrome/global/aboutSupport.properties. The l10n folks are OK with this?
Flags: needinfo?(rkothari)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14) > (In reply to Bob Owen (:bobowen) (less responsive until 26th) from comment > #12) > > [String/UUID change made/needed]: > > Adds new string property to > > toolkit/locales/en-US/chrome/global/aboutSupport.properties. > > The l10n folks are OK with this? This is about:support text so it should be fine even if we can't get it localized in time for 51.
(In reply to Jim Mathies [:jimm] from comment #15) > This is about:support text so it should be fine even if we can't get it > localized in time for 51. It's going to create noise in all tools and we've already done that a week ago for another bug. If you really want to land this in aurora, I'd strongly suggest to have a mozilla-aurora only patch with the string hard-coded.
(In reply to Francesco Lodolo [:flod] from comment #16) > (In reply to Jim Mathies [:jimm] from comment #15) > > This is about:support text so it should be fine even if we can't get it > > localized in time for 51. > > It's going to create noise in all tools and we've already done that a week > ago for another bug. > > If you really want to land this in aurora, I'd strongly suggest to have a > mozilla-aurora only patch with the string hard-coded. Bob, can you put this together? We don't want to roll out our first windows sandbox without having something in about:support.
Flags: needinfo?(bobowen.code)
Aurora uplift patch. The strings were pulled into the table in a data driven way so this fix is a little messy, so I felt I should get a new r+. MozReview-Commit-ID: HFRiEbkEztp
Attachment #8786767 - Flags: review?(dtownsend)
Flags: needinfo?(bobowen.code)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14) > (In reply to Bob Owen (:bobowen) (less responsive until 26th) from comment > #12) > > [String/UUID change made/needed]: > > Adds new string property to > > toolkit/locales/en-US/chrome/global/aboutSupport.properties. > > The l10n folks are OK with this? Sorry I missed this. Looks like l10n is Ok with hard-coding this string. Also, given that for Fx50 is the first e10s sandbox for mac and windows, I believe fixing this bug becomes critical. Please let me know if I am not connecting the dots correctly here.
Flags: needinfo?(rkothari)
Attachment #8786767 - Flags: review?(dtownsend) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: