1259601 - Add sandbox status to about:support (Windows)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Jim Mathies [:jimm]]

AFAICT we don't have anything in about:support about sandbox state on Windows.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=956ea4fff678

Comment 2

[Bob Owen (:bobowen)]

Attachment: Add content process sandbox level to about:support sandboxing information

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

Comment 3

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57769d38aafd67670663a76d8a651c438591b40b

Comment 5

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

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?

Comment 6

[Bob Owen (:bobowen)]

(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

Comment 7

[Bob Owen (:bobowen)]

(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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03d58bbdfc34
Add content process sandbox level to about:support sandboxing information. r=jld, r=mossop

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/03d58bbdfc34

Comment 10

[Tanvir Rahman]

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]

Comment 11

[Sylvestre Ledru [:Sylvestre]]

I don't think we want to uplift that.

Comment 12

[Bob Owen (:bobowen)]

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.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8780834 [details] [diff] [review]
Add content process sandbox level to about:support sandboxing information

Fix was verified on Nightly, Aurora50+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 15

[Jim Mathies [:jimm]]

(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.

Comment 16

[Francesco Lodolo [:flod]]

(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.

Comment 17

[Jim Mathies [:jimm]]

(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.

Comment 18

[Bob Owen (:bobowen)]

Attachment: Aurora patch - Add content process sandbox level to about:support sandboxing information

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

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6b75b2a627b