[Mac] Add a test that renders fonts from non-standard directories

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: haik, Assigned: haik)

Tracking

58 Branch
mozilla62
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: sb+)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

a year ago
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

a year ago
Assignee: nobody → haftandilian
Priority: -- → P2
(Assignee)

Comment 1

a year 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)

Updated

a year ago
See Also: → 1393259
(Assignee)

Comment 3

a year 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

a year ago
Whiteboard: sb+
(Assignee)

Updated

a year ago
Priority: P2 → P1
(Assignee)

Comment 4

a year 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

a year 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)

Updated

a year ago
Depends on: 1393259
(Assignee)

Updated

11 months ago
See Also: → 1460917
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months 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

11 months ago
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?
(Assignee)

Comment 11

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c869e76b79c5
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.