Closed Bug 512566 Opened 15 years ago Closed 12 years ago

[@font-face] font data urls should be loaded synchronously

Categories

(Core :: Layout: Text and Fonts, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: paulirish, Assigned: jtd)

References

()

Details

Attachments

(3 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/530.5 (KHTML, like Gecko) Chrome/2.0.172.39 Safari/530.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

I'm working on a robust method to detect @font-face support across browsers. (http://paulirish.com/2009/font-face-feature-detection/)

Firefox 3.5 is offering inconsistent results than the other supporting browsers (Saf4,Opera10).

The root of it is this, in pseudocode

var oldwidth = elem.offsetWidth;
elem.style.fontFamily = 'atfontfacefont';
var newwidth = elem.offsetWidth;
return oldwidth !== newwidth;

This should return true, indicating the new font has changed the display of the text. In Firefox 3.5, newwidth has the exact same value has oldwidth.

However if you check elem.offsetWidth again in a setTimeout (with 0ms delay) loop, the value will be updated and different.

This appears to be a bug as this behavior is not consistent with installed (local) fonts.


I should mention: in the testcase, the font data is included by way of a data: URI, as to make sure there is no external dependency.

Reproducible: Always

Steps to Reproduce:
1. check offsetWidth of an element
2. change element's font to an @font-face asset
3. check offsetWidth again.
Actual Results:  
width does not change.

Expected Results:  
width will likely be different.
Yeah, all our font loading code goes through an async load, including data urls which don't need to.  This means that rendered text goes through a (1) start download of font (2) render with fallback (3) on download completion, reflow to pick up downloaded font, even though data urls should really immediately load the font and skip the reflow steps.

I think it makes sense to test for data urls and do the load immediately.
Summary: @font-face application not considered during a reflow → [@font-face] font data urls should be loaded synchronously
Assignee: nobody → jdaggett
The existing code does an async load of data urls:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#305

Proposed solution:

1. Read data urls synchronously, as in the code below:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#776

2. Clean up the interface to gfxUserFontSet::OnLoadComplete.  Linux code uses aLoader as a lock on the memory pointed to by aFontData.  
We should clean that up and make that association clearer.

3. Rework the code so that the OnLoadComplete code can be called from within nsUserFontSet::StartLoad, some of the pointer
swizzling that's in StartLoad now will get tripped up by the code in OnLoadComplete.
Status: UNCONFIRMED → NEW
Ever confirmed: true
> 1. Read data urls synchronously, as in the code below:

No.  You really really don't want to do that.  All that does is do an async load under the hood and spin the event loop until the load completes.  That's not acceptable in the places where we load font data, last I checked.

I see no reason to support synchronous loading of font data.  Isn't the real issue in comment 0 that our font loading is completely lazy?
> Isn't the real issue in comment 0 that our font loading is completely lazy?

I'm not clear on the difference between asynchronous and lazy, but addressing the lazy load would certainly satisfy my original concern.
> I'm not clear on the difference between asynchronous and lazy

Asynchronous: the load starts, then the function that started the load returns, then the load finishes some time later.

Lazy: the load doesn't start until the font data is actually needed for the first time.

In particular, in your case it sounds like nothing on the page uses the "atfontfacefont" family until your JS runs, so the load doesn't start until the JS runs.  If something did use that font family, then the font would be loaded before onload fires.
(In reply to comment #3)
> > 1. Read data urls synchronously, as in the code below:
> 
> No.  You really really don't want to do that.  All that does is do an async
> load under the hood and spin the event loop until the load completes.  That's
> not acceptable in the places where we load font data, last I checked.

Why is that a bad idea exactly?  You're adding an extra step that serves no purpose (i.e. the whole start load/update event/restyle/load cycle).  Is your concern that it's a big chunk of work that should be deferred until after the load process?

> I see no reason to support synchronous loading of font data.  Isn't the real
> issue in comment 0 that our font loading is completely lazy?

Right, @font-face fonts are intended to be loaded lazily as needed.  That's the same across browsers, it's not particular to Gecko (er, with the exception of IE 6/7/8 which tries immediately to pull down all font data).
> Why is that a bad idea exactly? 

Because it spins the event loop.  What that means is that under the Load() call, other networks loads will proceed, events (mousemove, timeouts, etc) will fire, script will execute, etc.  And I'd be really surprised if the callers of FindFontEntry deal with that.

> That's the same across browsers

OK.  So for the testcase in comment 0, do Safari and Opera fail if the URI used is http: instead of data:?
First pass at reading in font data url's immediately, this will eliminate the flash of fallback font text for @font-face font's using data URL's.  I will solve Paul's original problem but that may not be something that will work cross-browser if other browsers are doing all font loads async.

Test with sites using font data URLs:

  www.typekit.com
  www.rypple.com

Still need to verify the logging is right in all cases and that the error handling is correct.  Needs reftests/mochitests.

Boris, I stepped through the load code, I don't see a spin/wait cycle in there but maybe I missed something.  The load code is in nsUserFontSet::LoadDataFont.
Hmm.  You're relying on implementation details of nsBaseChannel and nsDataChannel that are subject to change.  It happens to work now, but necko changes can break that at any time (including as e10s work progresses)....  In general, Open() is allowed to spin the event loop as desired.

I'd still like an answer to my question at the end of comment 7.
> Hmm.  You're relying on implementation details of nsBaseChannel and
> nsDataChannel that are subject to change.  It happens to work now,
> but necko changes can break that at any time (including as e10s work
> progresses)....  In general, Open() is allowed to spin the event
> loop as desired.

Ok, then that sounds like a bug that applies already to
nsFaviconService::SetFaviconDataFromDataURL.  Is there another way
to do this that doesn't involve spinning the event loop?  I don't
see why this needs to be so complex.

> I'd still like an answer to my question at the end of comment 7.

I haven't tested this yet but I don't think that's the main motivation
for doing this.  The main reason is to avoid the extra reflow that's
involved with layout with fallback font, load font, reflow to draw
with loaded font.
> that sounds like a bug that applies already to
> nsFaviconService::SetFaviconDataFromDataURL

Does that function guarantee not spinning the event loop?

> Is there another way to do this that doesn't involve spinning the event loop? 

That guarantees not spinning it?  Not really.

> I don't see why this needs to be so complex.

Because your code wants to treat different URI schemes very differently whereas the rest of the system tries to treat all URI schemes as similarly as generally possible to reduce the number of codepaths and complexity....
Would one alternative be to add an interface to datauris that lets you get the data out of them? And document that it's expected that that function does not spin the event loop?

Though I do sort of agree with John. It seems like it should always be possible to implement synchronous load from data urls without spinning the event loop. However guaranteeing that by adding tests would of course help with preventing accidental regressions.
Attachment #441446 - Attachment is obsolete: true
Attachment #575379 - Flags: review?(jfkthame)
Includes a check for URI_DOESNT_SPIN_EVENT_LOOP flag on the protocol handler.
Attachment #575380 - Flags: review?(bzbarsky)
Test both simple data url loads (good and bad) and delayed loads (good and bad)
Attachment #575382 - Flags: review?(jfkthame)
Comment on attachment 575380 [details] [diff] [review]
patch, part 2, font data url parsing

>+nsUserFontSet::ParseDataFont(gfxFontEntry *aFontToLoad,

So it may make more sense to structure the calling code from part 1 (which currently does GetScheme and then checks for "data") like so:

  if (NS_URIChainHasFlags(currSrc.mURI, 
                          nsIProtocolHandler::URI_SYNC_LOAD_DOESNT_SPIN_EVENT_LOOP)) {
    // Call ParseDataFont here
  }

This will make things work for any protocol that has that flag, not just data:.

And maybe name this function GetSyncLoadableFontData, since that's what it's doing.

>+  // check same-site origin

This is duplicating various code from nsUserFontSet::StartLoad, right?  I would much rather we refactor things so that the code is shared.  You could just have a helper function that runs all the code up to the bit after CheckLoadAllowed and call it from gfxUserFontSet::LoadNext before the branch on the URI flag above, I think.  If that function returns an error nsresult, then do the same thing that happens on error return from StartLoad right now.

>+  // use the data: protocol handler to convert the data
>+  nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsIProtocolHandler> protocolHandler;
>+  rv = ioService->GetProtocolHandler("data", getter_AddRefs(protocolHandler));
>+  NS_ENSURE_SUCCESS(rv, rv);

This whole bit could go away; you can still assert the presence of the URI_SYNC_LOAD_DOESNT_SPIN_EVENT_LOOP flag on aFontFaceSrc->mURI, of course.

>+  rv = protocolHandler->NewChannel(aFontFaceSrc->mURI, getter_AddRefs(channel));

And here you can just use 

  NS_NewChannel(aFontFaceSrc->mURI, getter_AddRefs(channel));

>+  // blocking stream is OK for data URIs

More precisely for the URIs we have the above flag set for.

>+  PRUint32 numRead;
>+  rv = stream->Read(reinterpret_cast<char*>(aBuffer), aBufferLength, &numRead);
>+  if (NS_SUCCEEDED(rv) && numRead == aBufferLength) {
>+    nsCAutoString mimeType;
>+    rv = channel->GetContentType(mimeType);
>+  } else if (numRead != aBufferLength) {
>+    rv = NS_ERROR_FAILURE;
>+  }

This should probably be a loop calling Read() until either 0 bytes are read (which indicates EOF) or you've read aBufferLength bytes or an exception is thrown....  Nothing guarantees that a Read() on a stream will return all available (as claimed by Available()) bytes.

>+++ b/netwerk/base/public/nsIProtocolHandler.idl
>-[scriptable, uuid(15fd6940-8ea7-11d3-93ad-00104ba0fd40)]
>+[scriptable, uuid(cd2f87b8-2822-47ba-9bc4-6a1426701cb1)]

You don't actually need to change the iid, since this is a source and binary compatible change.  In fact, it's better to not change it in this case.

>+     * URIs for this protocol don't need to spin event loop while loading.

Probably better to say:

  * Channels for this protocol don't need to spin the event loop to handle Open() and reads on the
  * resulting stream.

>+    const unsigned long URI_DOESNT_SPIN_EVENT_LOOP = (1<<15);

And I do think this should be URI_SYNCL_LOAD_DOESNT_SPIN_EVENT_LOOP.
Attachment #575380 - Flags: review?(bzbarsky) → review-
Attachment #575379 - Flags: review?(jfkthame)
Attachment #575382 - Flags: review?(jfkthame)
Attachment #575379 - Attachment is obsolete: true
Attachment #575380 - Attachment is obsolete: true
Attachment #580330 - Attachment is obsolete: true
Attachment #603977 - Flags: review?(jfkthame)
Updated based on review comments.
Attachment #580331 - Attachment is obsolete: true
Attachment #603978 - Flags: review?(bzbarsky)
Attachment #603977 - Attachment description: patch, part 1v2 - load font data urls synchronously (gfx portion) → patch, part 1v2a - load font data urls synchronously (gfx portion)
Attachment #575382 - Flags: review?(jfkthame)
I'm not sure that URI_SYNC_LOAD_DOESNT_SPIN_EVENT_LOOP is entirely the distinction we are looking for here. Loading from file also doesn't spin the event loop but we don't want to syncload fonts from file.

Maybe something like URI_SYNC_LOAD_IS_FROM_MEMORY or URI_SYNC_LOAD_IS_FAST or some such?
(In reply to Jonas Sicking (:sicking) from comment #21)
> Maybe something like URI_SYNC_LOAD_IS_FROM_MEMORY or URI_SYNC_LOAD_IS_FAST
> or some such?

Boris?  I'm fine with any of these.
or URI_SYNC_LOAD_IS_OK_FROM_UI_THREAD
Hmm.  All loads are on the UI thread, so it's more like URI_SYNC_LOAD_IS_OK for that last one.  I do think the "doesn't spin event loop" part is important, but I agree that so is the fact that we don't have to do I/O.

Maybe we should in fact just call this flag URI_SYNC_LOAD_IS_OK and clearly document what that means.
Comment on attachment 603978 [details] [diff] [review]
patch, part 2v2a - font data url parsing (network/loader portion)

>+nsUserFontSet::CheckFontLoad(gfxProxyFontEntry *aFontToLoad,
>+                             const gfxFontFaceSrc *aFontFaceSrc,
>+                             nsCOMPtr<nsIPrincipal>& aPrincipal)

Probably better to make aPrincipal an nsIPrincipal** like most out params.  That would make it clear in the callers that it's an out param.

>+  // blocking stream is OK for data URIs

I'd just remove that comment, or change it to talk about "for URIs that we decide to syncload" or something.

r=me with those changes and modulo whatever we decide about the flag.
Attachment #603978 - Flags: review?(bzbarsky) → review+
Comment on attachment 575382 [details] [diff] [review]
patch, part 3, font data url reftests

The tests are most likely fine, I expect, but this patch is not because it looks like you forgot to "hg add" the actual files!
Attachment #575382 - Flags: review?(jfkthame) → review-
Comment on attachment 603977 [details] [diff] [review]
patch, part 1v2a - load font data urls synchronously (gfx portion)

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

r+, modulo minor tweaks to eliminate reference parameters (see below) for better clarity, at least from LoadFont().

::: gfx/thebes/gfxUserFontSet.cpp
@@ +403,5 @@
>          userFontData->mMetaOrigLen = aMetaOrigLen;
>      }
>  }
>  
> +struct WOFFHeader {

This was moved in another bug, IIRC, so will no longer be needed here.

@@ +460,5 @@
>                                 nsresult aDownloadStatus)
>  {
>      // download successful, make platform font using font data
>      if (NS_SUCCEEDED(aDownloadStatus)) {
> +        gfxFontEntry *fe = LoadFont(aProxy, aFontData, aLength);

Comment that LoadFont is responsible to take ownership of the data, and set aFontData to null immediately after this call to ensure that we don't do anything more with it here.

@@ +549,5 @@
> +                       nsIProtocolHandler::URI_SYNC_LOAD_DOESNT_SPIN_EVENT_LOOP,
> +                       &loadDoesntSpin);
> +
> +                if (NS_SUCCEEDED(rv) && loadDoesntSpin)
> +                {

Paren goes on the same line as "if".

@@ +559,5 @@
> +                                          bufferLength);
> +
> +                    const PRUint8 *buf2 = buffer;
> +                    if (NS_SUCCEEDED(rv) &&
> +                        LoadFont(aProxyEntry, buf2, bufferLength)) {

Once LoadFont doesn't take reference parameters, the extra buf2 variable will no longer be needed, AFAICS.

@@ +733,5 @@
> +        ReplaceFontEntry(aProxy, fe);
> +
> +    } else {
> +
> +#ifdef PR_LOGGING

Lose the surplus blank lines before and after the #ifdef'd block here. And the one before } else {.

::: gfx/thebes/gfxUserFontSet.h
@@ +271,5 @@
> +    // returns font entry if platform font creation successful
> +    // Ownership of aFontData is passed in here; the font set must
> +    // ensure that it is eventually deleted with NS_Free().
> +    gfxFontEntry* LoadFont(gfxProxyFontEntry *aProxy,
> +                           const PRUint8* &aFontData, PRUint32 &aLength);

Let's not have reference parameters here; they tend to obscure what's happening at the call site. For aLength, I don't see that it was needed at all; for aFontData, I guess the point was so that LoadFont could set it to null, thus signalling the caller that it should not delete the buffer.

Instead of using references here, though, I think the caller should just set its pointer to null immediately after calling LoadFont.

@@ +277,5 @@
> +    // parse data for a data URL
> +    virtual nsresult SyncLoadFontData(gfxProxyFontEntry *aFontToLoad,
> +                                      const gfxFontFaceSrc *aFontFaceSrc,
> +                                      PRUint8* &aBuffer,
> +                                      PRUint32 &aBufferLength)

For reference parameters, it's usual to attach the & to the type rather than the parameter name, I think.

(I'm somewhat tempted to say we should use pointers rather than references, to make it clearer at the call site that they're out-params. But I won't insist on that, if Boris was ok with it in the other patch.)
Attachment #603977 - Flags: review?(jfkthame) → review+
> The tests are most likely fine, I expect, but this patch is not because it
> looks like you forgot to "hg add" the actual files!

Doh!
Attachment #575382 - Attachment is obsolete: true
Attachment #604906 - Flags: review?(jfkthame)
Hmm.  I assumed the out params were references to better fit into the caller.  If not, I too would prefer they were pointers.
Comment on attachment 604906 [details] [diff] [review]
patch, part 3, font data url reftests (really)

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

Hmm. I'd really prefer if the font data url tests didn't rely on -moz-font-feature-settings like this. That makes them inherently mozilla-specific, which seems a pity if it can be avoided (obviously, there are times when we're testing a mozilla-specific feature, but in general reftests ought to work across browsers), and it means they're going to need maintenance when we modify the syntax of -moz-font-feature-settings and/or unprefix the property. Surely we can test this without relying on OpenType features to recognize whether the font has been loaded OK?
Reftests updated to remove dependence on font feature use.
Attachment #604906 - Attachment is obsolete: true
Attachment #605275 - Flags: review?(jfkthame)
Attachment #604906 - Flags: review?(jfkthame)
Attachment #605275 - Attachment description: patch, part 3 - font data url reftests → patch, part 3v2 - font data url reftests
Comment on attachment 605275 [details] [diff] [review]
patch, part 3v2 - font data url reftests

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

It would've been nice to subset the font down to just ASCII (or even further), to reduce the bulk somewhat, but whatever.....

::: layout/reftests/font-face/reftest.list
@@ +30,5 @@
>  fails-if(Android) == src-list-local-full.html src-list-local-full-ref.html
>  fails-if(Android) == src-list-local-full-quotes.html src-list-local-full-ref.html
>  HTTP(..) == src-list-local-fallback.html src-list-local-fallback-ref.html
>  
> +# data url tests (these don't need the HTTP server

Unmatched paren.

@@ +37,5 @@
> +== src-list-data-3.html src-list-data-ref.html
> +== src-list-data-4.html src-list-data-ref.html
> +
> +# load with data url vs. font data load
> +HTTP(..) == src-list-data-1.html src-list-actual-font-ref.html

I'd suggest using src-list-data-ref.html on the left-hand side here, so that it's easier to tell which test is failing (as the report will indicate the first filename) if something ever breaks.
Attachment #605275 - Flags: review?(jfkthame) → review+
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: