1092570 - CSSFontFaceSet exposed to the web by default

Status: RESOLVED FIXED

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

Description

[Louis-Rémi BABE]

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/

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Louis-Rémi BABE]

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.

Comment 3

[Cameron McCormack (:heycam)]

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.)

Comment 4

[Cameron McCormack (:heycam)]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

[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.

Comment 6

[Cameron McCormack (:heycam)]

(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.

Comment 7

[John Daggett (:jtd)]

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. :)

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 9

[Boris Zbarsky [: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

Comment 10

[Cameron McCormack (:heycam)]

(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.

Comment 11

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=c653f40f1cc7

Comment 12

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e4ac541c08

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5e4ac541c08

Comment 14

[Boris Zbarsky [:bzbarsky]]

Please don't forget to backport to 35.

Comment 15

[Cameron McCormack (:heycam)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ca49027a199