Closed Bug 1092570 Opened 5 years ago Closed 5 years ago

CSSFontFaceSet exposed to the web by default

Categories

(Core :: DOM: CSS Object Model, defect)

35 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 + fixed

People

(Reporter: public, Assigned: heycam)

References

()

Details

Attachments

(1 file)

on some pages, such as http://dev.w3.org/csswg/css-font-loading/ or http://nodebox.github.io/opentype.js/ , opening the console and typing window.FontFace returns undefined. window.FontFaceSet is always available.

Doing the same on bugzilla (and most webpages out there) will return function, as expected.

I haven't been able to produce a reduced test-case yet. window.FontFace also returns undefined in a page as simple as http://www.perdu.com/
Blocks: 1088437
FontFace is conditionally exposed based on the "layout.css.font-loading-api.enabled" preference.  This preference is set to false by default.  So undefined is what you should expect; the API is not enabled.

FontFaceSet is also exposd conditional on the same preference, but the thing is, the preference only controls whether the constructor is visible via direct lookups.  If you create an object implementing the interface, that will define the constructor unconditionally.  We don't expect to have situation where objects implementing an interface are being thrown around but the interface is not exposed.

So are we supposed to be exposing this stuff or not?
Flags: needinfo?(jdaggett)
Alright. But what's strange is that FontFace IS available on Bugzilla and many other pages out there. The console returns "function" but I haven't actually tried to use it.
A FontFace object gets created for every @font-face rule that gets used.  Boris says in comment 1 that creating an object will cause the constructor to exist, regardless of the pref, so that would be why.

Note that the FontFace that gets created but shouldn't be accessible to script if the pref isn't set.  (Although now that I think about it we are exposing them on CSSFontFaceLoadEvents -- we ought to check the pref before dispatching those events.)
(In reply to Cameron McCormack (:heycam) (away October 6 - November 9) from comment #3)
> (Although now that I think about it we are exposing them on CSSFontFaceLoadEvents -- we ought to
> check the pref before dispatching those events.)

Filed bug 1093022.
Flags: needinfo?(cam)
[Tracking Requested - why for this release]:  Exposing stuff that we shouldn't be.

> Boris says in comment 1 that creating an object will cause the constructor to exist

More precisely when its JS reflection is created.  In the case of FontFaceSet, this happens in the FontFaceSet constructor, which resolves mReady with "this", causing "this" to get JS-wrapped (by the way, this is operating on a zero-refcount object, which is a really bad idea; I suggest moving that stuff out to an init function or something).  We should probably condition the promise creation on the pref so we don't wrap the FontFaceSet when the pref is not set.

Also, do we unconditionally set this pref in the test harness or something?  Because otherwise test_interfaces.html should have caught this....

As far as why FontFace is sometimes exposed, it happens in FontFace::SetStatus when mLoaded is resolve with "this".  Again, we should condition that on the pref.
Summary: Some pages don't expose the CSS Font Loading API → CSSFontFaceSet exposed to the web by default
(In reply to Boris Zbarsky [:bz] from comment #5)
> Also, do we unconditionally set this pref in the test harness or something? 
> Because otherwise test_interfaces.html should have caught this....

Yeah I'm setting it in testing/profiles/prefs_general.js.
Flags: needinfo?(jdaggett)
Not really sure what should and shouldn't be exposed to JS but the FontFaceSet and FontFace objects are used when @font-face rules are present regardless of the pref setting.

For now, I'm going to assume that Cam will tackle this next week when he's back. :)
Flags: needinfo?(cam)
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Flags: needinfo?(cam)
Attachment #8519614 - Flags: review?(bzbarsky)
Comment on attachment 8519614 [details] [diff] [review]
patch

I assume all this stuff is mainthread only?

You should probably use a pref cache in PrefEnabled, since this will be called for each font face...

r=me
Attachment #8519614 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #9)
> I assume all this stuff is mainthread only?

Yes.

> You should probably use a pref cache in PrefEnabled, since this will be
> called for each font face...

I should.
https://hg.mozilla.org/mozilla-central/rev/c5e4ac541c08
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please don't forget to backport to 35.
Flags: needinfo?(cam)
Comment on attachment 8519614 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1028497
[User impact if declined]: preffed off feature can still sometimes be visible in a page
[Describe test coverage new/current, TBPL]: been on m-c for less than a day
[Risks and why]: very low, we're just skipping some code that isn't useful when the pref is off anyway
[String/UUID change made/needed]: N/A
Flags: needinfo?(cam)
Attachment #8519614 - Flags: approval-mozilla-aurora?
Attachment #8519614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.