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

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

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.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: 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

Comment 3

[Cameron McCormack (:heycam)]

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

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Cameron McCormack (:heycam)]

(I'm going to investigate further, as I'd expect the cycle collector to be enough here.)

Comment 6

[Cameron McCormack (:heycam)]

Turns out I wasn't traversing/unlinking FontFaceSet::mUserFontSet->mFontFaceSet.

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=127445ed80be

Comment 8

[Cameron McCormack (:heycam)]

Attachment: 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?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Cameron McCormack (:heycam)]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Cameron McCormack (:heycam)]

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.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 1: Create FontFaceSet with a document rather than a pres context.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 2: Create FontFace objects with a FontFaceSet rather than a pres context.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 3: Only reflow from the font face loader if we have a pres context.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 4: Move FontFaceSet ownership from nsPresContext to nsIDocument.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 5: Cancel font loads on FontFaceSet destruction.

We used to do this on pres context destruction.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Part 6: Update test assertion annotations.

These no longer assert for the reasons mentioned in comment 3.

Comment 20

[Cameron McCormack (:heycam)]

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

Comment 21

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc45a10546d

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Does that end up exposing GC/CC behavior to web pages?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

or was that just about the leak you fixed the other day?

Comment 26

[Cameron McCormack (:heycam)]

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

Comment 27

[Cameron McCormack (:heycam)]

(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

[John Daggett (:jtd)]

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?

Comment 29

[John Daggett (:jtd)]

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)

Comment 30

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfa257a4564

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

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