Closed
Bug 1432567
Opened 6 years ago
Closed 6 years ago
[Mac] Add a test that renders fonts from non-standard directories
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: haik, Assigned: haik)
References
Details
(Whiteboard: sb+)
Attachments
(1 file, 5 obsolete files)
On Mac, many users use 3rd party font managers which can store fonts on the filesystem anywhere the user chooses. We should have a test that installs a font from a non-standard directory and then renders a page using that font, making sure the font is displayed correctly.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → haftandilian
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
A CLI command that registers or unregisters fonts in place. Using this command, we could register a font from a non-standard location, display a page that depends on this font, validate the page is displayed correctly, then uninstall the font. Any directory not included in our content sandbox read-access whitelist is considered a non-standard font directory. For example, ~/MyFonts/font.otf.
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Register/unregister fonts in python to make this easier to use in automated tests--no compilation required.
Attachment #8944816 -
Attachment is obsolete: true
Attachment #8944817 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Whiteboard: sb+
Assignee | ||
Updated•6 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 4•6 years ago
|
||
Work-in-progress patch that adds a test that checks that a page using the Fira Sans font displays correctly when the font is installed in a standard directory readable from content processes AND a directory not readable from content processes. Fira Sans is not included with macOS but is already in the tree. The test passes locally on 10.13, but fails on try with 10.10. In addition to the try server failure, some synchronization and other improvements are still needed.
Attachment #8944874 -
Attachment is obsolete: true
Assignee | ||
Comment 5•6 years ago
|
||
In debugging the 10.10 try failure referenced in comment 4, I found that the font, when installed in a private directory (that content doesn't have read access to) is picked up by content and the sandbox extension mechanism is working. I have not yet confirmed why the test fails. It may be due to a race condition in how the test waits for a font to be installed before testing it is displayed by content. The test uses a prefObserver with "font.internaluseonly.changed" to wait until installing the font in the OS is detected by the browser, but the pref notification occurs in the parent and test doesn't wait for any indication the new font has been propagated to the child. Needs more debugging.
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8954500 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
With help from Jonathan, I've updated the patch to not use snapshots but instead to check for the LastResort font being used. The test registers a font from the root of the home directory which is not normally accessible to content processes. As a follow-up, the test could be updated to test all the cases we are currently whitelisting in SandboxPolicies.h for 10.11 and earlier.
Assignee | ||
Updated•6 years ago
|
Attachment #8979155 -
Attachment is obsolete: true
Assignee | ||
Comment 9•6 years ago
|
||
Before some refactoring: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dbf4113a834136dcee2e3ff23102c2048dd98ae Current code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3efe68ca36a765dd2d32176d04b9e761225b0d64
Comment 10•6 years ago
|
||
Looks reasonable. Just to check, have you verified that the test actually fails on a revision prior to bug 1460917?
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #10) > Looks reasonable. Just to check, have you verified that the test actually > fails on a revision prior to bug 1460917? I tested locally but didn't think to submit that to try. I submitted one this morning with the fix for bug 1460917 backed out and it fails in the right way (because the try builds run on 10.10 which is affected by bug 1460917): TEST-UNEXPECTED-FAIL | security/sandbox/test/browser_bug1393259.js | The LastResort fallback font was not used - false == true - JS frame :: chrome://mochitests/content/browser/security/sandbox/test/browser_bug1393259.js :: <TOP_LEVEL> :: line 160 TEST-UNEXPECTED-FAIL | security/sandbox/test/browser_bug1393259.js | The test font "Fira Sans" was used - false == true - JS frame :: chrome://mochitests/content/browser/security/sandbox/test/browser_bug1393259.js :: <TOP_LEVEL> :: line 163 https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d03dd107fd01f38cb7860fe2c5e05fe35b2e15
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8979742 [details] Bug 1432567 - [Mac] Add a test that renders fonts from non-standard directories https://reviewboard.mozilla.org/r/245888/#review252582 ::: security/sandbox/test/browser.ini:11 (Diff revision 1) > + mac_register_font.py > + ../../../layout/reftests/fonts/fira/FiraSans-Regular.otf > > -skip-if = !e10s > [browser_content_sandbox_fs.js] > -skip-if = !e10s || (debug && os == 'win') # bug 1379635 > +skip-if = !e10s The "skip-if (debug && os=='win')" needs to be applied to browser_content_sandbox_fs.js and my changes broke this. Will fix.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8979742 [details] Bug 1432567 - [Mac] Add a test that renders fonts from non-standard directories https://reviewboard.mozilla.org/r/245888/#review252600 OK, thanks for checking it out on tryserver. LGTM, with the above issue fixed.
Attachment #8979742 -
Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8979742 [details] Bug 1432567 - [Mac] Add a test that renders fonts from non-standard directories https://reviewboard.mozilla.org/r/245888/#review252600 Thanks for the review.
Comment 16•6 years ago
|
||
Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c869e76b79c5 [Mac] Add a test that renders fonts from non-standard directories r=jfkthame
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c869e76b79c5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•