Closed
Bug 1259601
Opened 9 years ago
Closed 8 years ago
Add sandbox status to about:support (Windows)
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 51
People
(Reporter: jimm, Assigned: bobowen)
References
Details
(Whiteboard: sbwc1)
Attachments
(2 files)
(deleted),
patch
|
jld
:
review+
mossop
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
AFAICT we don't have anything in about:support about sandbox state on Windows.
Reporter | ||
Updated•9 years ago
|
Whiteboard: sb? → sbwc1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Updated•8 years ago
|
Attachment #8780834 -
Flags: review?(dtownsend) → review+
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
(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
Assignee | ||
Comment 7•8 years ago
|
||
(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
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
Blocks: about:support
Comment 10•8 years ago
|
||
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•8 years ago
|
||
I don't think we want to uplift that.
Assignee | ||
Comment 12•8 years ago
|
||
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-firefox50:
--- → affected
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+
Comment 14•8 years ago
|
||
(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)
Reporter | ||
Comment 15•8 years ago
|
||
(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•8 years ago
|
||
(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.
Reporter | ||
Comment 17•8 years ago
|
||
(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)
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
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)
Updated•8 years ago
|
Attachment #8786767 -
Flags: review?(dtownsend) → review+
Comment 20•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•