Closed Bug 1259601 Opened 4 years ago Closed 3 years ago

Add sandbox status to about:support (Windows)

Categories

(Firefox :: General, defect)

defect
Not set

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)
Duplicate of this bug: 1259602
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
https://hg.mozilla.org/mozilla-central/rev/03d58bbdfc34
Status: ASSIGNED → RESOLVED
Closed: 3 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?
Status: RESOLVED → VERIFIED
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.