make the Font Loading API work in a display:none iframe

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Following on from bug 1153230.

I think we should be able to make the Font Loading API work in a display:none iframe, at least in terms of being able to add new FontFace objects to it and start loading them (even if we won't have any frames that use them for rendering).  We won't be able to automatically include FontFace objects for @font-face rules since we don't have a style set to get them from.
(Assignee)

Comment 1

3 years ago
I wrote in Bug 1153230 comment 9:
> Most uses of the pres shell within FontFaceSet/FontFace are for getting at the document,
> and we do store a pointer to that on the FontFaceSet, so we're not terribly dependent on
> the pres shell.  The only one I'm not sure what to do with is the use of
> mPresContext->DeviceContext()->GetMetricsFor() I'll be adding in bug 1072102.

I guess we could crawl up until we find a document with a pres shell and use its device context, in this case.
(Assignee)

Comment 2

3 years ago
Created attachment 8602544 [details] [diff] [review]
WIP v0.1

This moves FontFaceSet ownership from nsPresContext to nsIDocument.

I get a leak in one of the devtools tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82cb48a5f3ae
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
The loss of assertions in 1137723-1.html is because we no longer need to create a new FontFaceSet when we create a new pres shell/context for the print preview of the document.  (We should, though, flush the FontFaceSet since we might have a different set of @font-face rules.  I don't think it's possible currently for @font-face rules to be inside @media rules, but it's conceivable that they set of style sheets could be changed.)
(Assignee)

Comment 4

3 years ago
In my patch, I make nsDocument have a strong reference to its FontFaceSet and vice versa.  I want to ensure that if you write, say

  var f = (new DOMParser()).parseFromString("...", "text/html").fonts;

and hold on to it, that the FontFaceSet will continue to work properly even though script hasn't held on to the document.  The FontFaceSet needs various things from the document if it starts a font load (such as its load group, node principal, etc.).

Similarly, it seems like we should be returning the same FontFaceSet object each time from .fonts.

So that is where the leak from the patch is from.  Olli, do you have any suggestions on how to deal with this?  I did have a call to mFontFaceSet->DestroyUserFontSet() in nsDocument::Destroy, which severs the connection between the two, but that only works for documents which are presented, and in any case if the document is no longer being presented but script still has a hold on the document or FontFaceSet then I actually don't want to sever that connection.
Flags: needinfo?(bugs)
(Assignee)

Comment 5

3 years ago
(I'm going to investigate further, as I'd expect the cycle collector to be enough here.)
Flags: needinfo?(bugs)
(Assignee)

Comment 6

3 years ago
Turns out I wasn't traversing/unlinking FontFaceSet::mUserFontSet->mFontFaceSet.
(Assignee)

Updated

3 years ago
Depends on: 1162855
(Assignee)

Updated

3 years ago
Depends on: 1163446
(Assignee)

Comment 7

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=127445ed80be
(Assignee)

Comment 8

3 years ago
Created attachment 8603952 [details] [diff] [review]
WIP v0.2

I think this is working now, for display:none iframes that are in the document.

What doesn't work is:
  (a) a document that was in an iframe, but whose iframe was subsequently
      removed from the document
  (b) a document returned from createHTMLDocument or similar

The issue with these is that the document has no script global, and thus we can't pass in a global object to the FontFaceSet (or FontFace) constructors.  The global object is really only used to be able to create Promise objects.  The rest of the API works just fine, as far as I can tell.

It doesn't look there's any way to create and use Promise objects without a global?

Is there a way to tell that a document was created for an iframe, and to go up to the ownerDocument (which hopefully does have a script global)?  Is that a good idea in any case?  Is nsIDocument::GetScopeObject anything useful here?

Not sure what to do with documents created with for example DOMParser or DOMImplementation.createDocument, either, assuming the response to https://lists.w3.org/Archives/Public/www-style/2015May/0091.html is that we should indeed have a FontFaceSet object for those.

Any suggestions Olli/Boris?
Attachment #8602544 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
(In reply to Cameron McCormack (:heycam) from comment #8)
 Is nsIDocument::GetScopeObject anything
> useful here?
Yes, use GetScopeObject.
(there is also GetScriptHandlingObject(), which returns the same object as GetScopeObject, but will return null earlier)


> 
> Not sure what to do with documents created with for example DOMParser or
> DOMImplementation.createDocument,
They have scopeObject (and also scriptHandlingObject)
Flags: needinfo?(bugs)
> It doesn't look there's any way to create and use Promise objects without a global?

Correct.  Of course once they move into SpiderMonkey this will become _really_ obvious.  ;)

Also, I would assume the font API spec specifies which global should be used for the relevant promises.  If it doesn't, it obviously needs to...

But yes, for now use GetScopeObject; that's what it's for.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 11

3 years ago
Thanks.  So this is what I am testing with:

  var f = document.createElement("iframe");
  f.style.display = "none";
  document.body.appendChild(f);
  var doc = f.contentDocument;
  f.remove();
  doc.fonts.ready.then(() => console.log("ready"));

Using doc's scope object does let me create the Promise object, but because that window's mIsDying == true (as a result of the f.remove() call), it never gets resolved, due to the check in Promise::Settle.

I just skimmed through bug 1058695.  Is this not running promise callback behaviour something sanctioned by a spec (or in the future will be)?  If so, it might be just fine to behave in this way for FontFaceSets from documents whose window was cleaned up.  All other aspects of the API should keep working.  Reckon that's OK?
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

3 years ago
Blocks: 1149381
> Is this not running promise callback behaviour something sanctioned by a spec (or in the
> future will be)?

I really hope so.  Because anything else requires that a browser never exits and leaks all memory forever.  Which, arguably _is_ what specs say right now, but it's daft and needs to stop.

> Reckon that's OK?

Yep.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 13

3 years ago
Great.  FYI, the FontFaceSet in a createHTMLDocument()-returned document isn't that useful currently, since we have a restriction that a FontFace can only be added to a FontFaceSet if they both came from the same global.  I plan to lift that restriction (and allow FontFace objects in multiple FontFaceSets, as the spec allows) in a later bug.  So at that point I'll add some more testing of such a FontFaceSet.
(Assignee)

Updated

3 years ago
Blocks: 1163877
(Assignee)

Comment 14

3 years ago
Created attachment 8604423 [details] [diff] [review]
Part 1: Create FontFaceSet with a document rather than a pres context.
Attachment #8603952 - Attachment is obsolete: true
Attachment #8604423 - Flags: review?(jdaggett)
(Assignee)

Comment 15

3 years ago
Created attachment 8604424 [details] [diff] [review]
Part 2: Create FontFace objects with a FontFaceSet rather than a pres context.
Attachment #8604424 - Flags: review?(jdaggett)
(Assignee)

Comment 16

3 years ago
Created attachment 8604425 [details] [diff] [review]
Part 3: Only reflow from the font face loader if we have a pres context.
Attachment #8604425 - Flags: review?(jdaggett)
(Assignee)

Comment 17

3 years ago
Created attachment 8604426 [details] [diff] [review]
Part 4: Move FontFaceSet ownership from nsPresContext to nsIDocument.
Attachment #8604426 - Flags: review?(bugs)
(Assignee)

Comment 18

3 years ago
Created attachment 8604428 [details] [diff] [review]
Part 5: Cancel font loads on FontFaceSet destruction.

We used to do this on pres context destruction.
(Assignee)

Updated

3 years ago
Attachment #8604428 - Flags: review?(jdaggett)
(Assignee)

Comment 19

3 years ago
Created attachment 8604429 [details] [diff] [review]
Part 6: Update test assertion annotations.

These no longer assert for the reasons mentioned in comment 3.
Attachment #8604429 - Flags: review?(jdaggett)
(Assignee)

Comment 20

3 years ago
Created attachment 8604430 [details] [diff] [review]
Part 7: Test Font Loading API in a display:none iframe.

For each sub-test in test_font_loading_api.html, we now run it first on the visible document/window then second on the display:none iframe's document/window.
Attachment #8604430 - Flags: review?(jdaggett)
(Assignee)

Updated

3 years ago
Blocks: 1163879
(Assignee)

Comment 21

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc45a10546d
Comment on attachment 8604426 [details] [diff] [review]
Part 4: Move FontFaceSet ownership from nsPresContext to nsIDocument.

>+  /*
>+  if (mFontFaceSet) {
>+    // Clear out user font set if we have one
>+    mFontFaceSet->DestroyUserFontSet();
>+    mFontFaceSet = nullptr;
>+  }
>+  */
We don't want to set mFontFaceSet to null here, right? And remove /* and */

>+++ b/dom/base/nsIDocument.h
update IID of nsIDocument

>+  gfxUserFontSet* GetUserFontSet();
>+  void FlushUserFontSet();
>+  void RebuildUserFontSet(); // asynchronously
>+  mozilla::dom::FontFaceSet* GetFonts() { return mFontFaceSet; }
Could you call this GetExistingFonts

>+  void HandleRebuildUserFontSet() {
{ goes to the next line

>+  // Is the current mFontFaceSet valid?
>+  unsigned              mFontFaceSetDirty : 1;
>+  // Has GetUserFontSet() been called?
>+  unsigned              mGetUserFontSetCalled : 1;
>+  // Do we currently have an event posted to call FlushUserFontSet?
>+  unsigned              mPostedFlushUserFontSet : 1;
Perhaps use bool here, and no need to have so much space between the type and variable name.
See the code above
Attachment #8604426 - Flags: review?(bugs) → review+
(Assignee)

Comment 23

3 years ago
(In reply to Olli Pettay [:smaug] from comment #22)
> >+  if (mFontFaceSet) {
> >+    // Clear out user font set if we have one
> >+    mFontFaceSet->DestroyUserFontSet();
> >+    mFontFaceSet = nullptr;
> >+  }
> >+  */
> We don't want to set mFontFaceSet to null here, right? And remove /* and */

Yeah that block should have been removed entirely.  (We now don't want to destroy the user font set when the pres context at this point -- instead we want to wait until it gets CCed.)
Does that end up exposing GC/CC behavior to web pages?
or was that just about the leak you fixed the other day?
(Assignee)

Comment 26

3 years ago
(In reply to Olli Pettay [:smaug] from comment #24)
> Does that end up exposing GC/CC behavior to web pages?

No it shouldn't; the only difference in behaviour when the pres context has disappeared should be in FontFaceSet::MightHavePendingFontLoads, where we no longer check to see if we have a pending restyle or reflow to delay the FontFaceSet from becoming status == "loaded", which is what we want to do.

(In reply to Olli Pettay [:smaug] from comment #25)
> or was that just about the leak you fixed the other day?

Fixing that leak allows me to remove this DestoryUserFontSet call, which really was papering over the fact that the CC macros weren't correct.
(Assignee)

Comment 27

3 years ago
(In reply to Cameron McCormack (:heycam) from comment #1)
> I guess we could crawl up until we find a document with a pres shell and use
> its device context, in this case.

I found it simpler to add the ability to create a gfxFontGroup without an nsDeviceContext; patches to come in bug 1072102 in the coming days.

Comment 28

3 years ago
Comment on attachment 8604423 [details] [diff] [review]
Part 1: Create FontFaceSet with a document rather than a pres context.

Review of attachment 8604423 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ -1174,5 @@
>                             nsIPrincipal** aPrincipal,
>                             bool* aBypassCache)
>  {
> -  // check same-site origin
> -  nsIPresShell* ps = mPresContext->PresShell();

leave the comment?
Attachment #8604423 - Flags: review?(jdaggett) → review+

Updated

3 years ago
Attachment #8604424 - Flags: review?(jdaggett) → review+

Updated

3 years ago
Attachment #8604425 - Flags: review?(jdaggett) → review+

Comment 29

3 years ago
Comment on attachment 8604428 [details] [diff] [review]
Part 5: Cancel font loads on FontFaceSet destruction.

Review of attachment 8604428 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with all references to DestroyUserFontSet cleaned out (other patches have this code commented out)
Attachment #8604428 - Flags: review?(jdaggett) → review+

Updated

3 years ago
Attachment #8604429 - Flags: review?(jdaggett) → review+

Updated

3 years ago
Attachment #8604430 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 30

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfa257a4564
(Assignee)

Updated

3 years ago
No longer blocks: 1149381

Comment 31

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57086b92200d
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f448e76abb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b95bf6004f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5c33b5e955
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a4d8018a6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d52317b1e54
https://hg.mozilla.org/integration/mozilla-inbound/rev/4847732dfb92
https://hg.mozilla.org/mozilla-central/rev/57086b92200d
https://hg.mozilla.org/mozilla-central/rev/68f448e76abb
https://hg.mozilla.org/mozilla-central/rev/a8b95bf6004f
https://hg.mozilla.org/mozilla-central/rev/cd5c33b5e955
https://hg.mozilla.org/mozilla-central/rev/09a4d8018a6f
https://hg.mozilla.org/mozilla-central/rev/2d52317b1e54
https://hg.mozilla.org/mozilla-central/rev/4847732dfb92
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

2 years ago
Blocks: 1153230
You need to log in before you can comment on or make changes to this bug.