Closed Bug 1161413 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(7 files, 2 obsolete files)

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.
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.
Attached patch WIP v0.1 (obsolete) — Splinter Review
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
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.)
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)
(I'm going to investigate further, as I'd expect the cycle collector to be enough here.)
Flags: needinfo?(bugs)
Turns out I wasn't traversing/unlinking FontFaceSet::mUserFontSet->mFontFaceSet.
Depends on: 1162855
Depends on: 1163446
Attached patch WIP v0.2 (obsolete) — Splinter Review
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)
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)
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)
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.
Blocks: 1163877
We used to do this on pres context destruction.
Attachment #8604428 - Flags: review?(jdaggett)
These no longer assert for the reasons mentioned in comment 3.
Attachment #8604429 - Flags: review?(jdaggett)
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)
Blocks: 1163879
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+
(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?
(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.
(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 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+
Attachment #8604424 - Flags: review?(jdaggett) → review+
Attachment #8604425 - Flags: review?(jdaggett) → review+
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+
Attachment #8604429 - Flags: review?(jdaggett) → review+
Attachment #8604430 - Flags: review?(jdaggett) → review+
No longer blocks: 1149381
Blocks: 1153230
You need to log in before you can comment on or make changes to this bug.