515240 - "ASSERTION: MakePlatformFont called with null data ptr"

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Loading http://www.machinima.org/machinima-faq.html triggers some assertions:

###!!! ASSERTION: MakePlatformFont called with null data ptr: 'aFontData && aLength != 0', file /Users/jruderman/central/gfx/thebes/src/gfxMacPlatformFontList.mm, line 699

###!!! ASSERTION: null font data: 'aFontData && aFontDataLength != 0', file /Users/jruderman/central/gfx/thebes/src/gfxFontUtils.cpp, line 702

WARNING: invalid font (insufficient data): file /Users/jruderman/central/gfx/thebes/src/gfxFontUtils.cpp, line 708

WARNING: downloaded font error, invalid font data for (screenfox9): file /Users/jruderman/central/gfx/thebes/src/gfxMacPlatformFontList.mm, line 710

Comment 1

[Jonathan Kew [:jfkthame]]

This looks pretty benign. The site links to a couple of EOT fonts, but doesn't provide a format hint, so our expected behavior would be to download them and then discover we don't support their format (generating an "invalid font data..." warning in a debug build).

However, it seems that the fonts are not actually present at that site, yet it doesn't respond with a 404 or similar error, it simply gives us a zero-byte response. So that zero-length chunk of data gets passed to MakePlatformFont when the load "completes", and that's triggering the assertions (but we then correctly determine that it's not a usable font, and continue).

We could either check earlier for a completely empty response to the font load request, or remove those non-zero length checks on the grounds that ValidateSfntHeaders will quickly reject a zero-length font anyway.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: minimized testcase that triggers these assertions

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: remove over-zealous assertions (zero length data is handled safely)

With this patch, the zero-length font data in the testcase (and the original URL) is handled correctly, resulting in two warnings, "invalid font (insufficient data)" and then "downloaded font error, invalid font data for (<fontname>)" but no assertions.

Comment 4

[John Daggett (:jtd)]

Comment on attachment 399353 [details] [diff] [review]
remove over-zealous assertions (zero length data is handled safely)

I think the asssertions are fine, the error should be handled one level up.

Comment 5

[John Daggett (:jtd)]

Attachment: patch, v.0.2, treat null data as download failure

No need to try to validate a null data ptr, just detect that and take the download failure case immediately.

Comment 6

[Jonathan Kew [:jfkthame]]

It doesn't seem right to treat a successful download of a zero-length file (which, as far as we can tell, is the case here) as being a download failure, which is what happens if you test for aLength != 0 here. Testing for non-null aFontData would be fine; if the pointer is null, we don't want to do anything further with it.

It's true that zero-length data cannot be a valid font, but that would also be true if the download is one byte long, or two, or three..... there's no reason to treat the zero-length case specially at this level. The first thing we do in ValidateSfntHeaders is to check that we've got enough data to meaningfully start examining it.

So I still think those assertions are "over-zealous". We don't need the assertion in gfxMacPlatformFontList::MakePlatformFont at all (and note that we don't have one in the equivalent place on other platforms), as gfxFontUtils::ValidateSFNTHeaders asserts exactly the same conditions; and the assertion there should check that the pointer is non-null (I left that part in place), but there is no reason for it to assert anything about aFontDataLength, that's simply part of what we check to find out whether we've got usable font data.

So please reconsider the patch I suggested; I'm arguing that there is (a) no need for the current duplication of the assertion, and (b) no reason to assert if the download has given us a valid data pointer that just happens to have too little data (including zero bytes) to be a usable font. That's something we handle correctly without an assertion.

If you want to check for that particular special case earlier as well, I suppose that's ok; however, the length == 0 case should be reported as "insufficent data" rather than as a download error. But I don't see any real benefit to adding that check in OnLoadComplete, given that we handle it fine already.

Comment 7

[Jonathan Kew [:jfkthame]]

This is resolved as a side-effect of bug 507970 (WOFF font support): the bad (zero-length) download is rejected by the new gfxUserFontSet.cpp function PrepareOpenTypeData, which determines that it cannot be valid font data. It prints an NS_WARNING (in debug builds), and the request is safely discarded.