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)

Product:
Core
Component:
Security: Process Sandboxing
Version:
58 Branch
Platform:
Unspecified
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

(Whiteboard: sb+)

Attachments

(1 file, 5 obsolete files)

Bug 1432567 - [Mac] Add a test that renders fonts from non-standard directories
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
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: nobody → haftandilian
Priority: -- → P2
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.
See Also: → 1393259
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
Whiteboard: sb+
Priority: P2 → P1
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
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.
Depends on: 1393259
See Also: → 1460917
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.
Attachment #8979155 - Attachment is obsolete: true
Looks reasonable. Just to check, have you verified that the test actually fails on a revision prior to bug 1460917?
(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
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 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 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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: