Closed Bug 1153230 Opened 9 years ago Closed 8 years ago

Github failed "pdf viewing" on Firefox desktop nightly


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

Not set





(Reporter: sotaro, Assigned: heycam)



(Keywords: qawanted, regression)


(1 file)

githab now support pdf viewing by using pdf.js

But on latest Firefox Desktop nightly, it failed to show pdf view. Example content is the following. I checked on two windows 7 laptops. But both failed to show the pdf view.
The problem happened also when e10s was disabled.
Summary: Githab failed "pdf viewing" on Firefox desktop nightly → Github failed "pdf viewing" on Firefox desktop nightly

Triggered by: Bug 1088437

setting layout.css.font-loading-api.enabled = false helps
Blocks: 1088437
Component: General → CSS Parsing and Computation
Version: unspecified → Trunk
So in a failing build, I get this in the console:

  NS_ERROR_FAILURE:  pdf-51bf5601989ef1cb4f75ad24d286835e.js:5:0

Not too helpful, given the obfuscated code in that JS file.

And yes, non-nightly channels are not affected.  That said, I get the failure on nightly builds going back to Dec 2014....  But I guess only in e10s mode.
Hoping Cameron could have a look, although without any investigation whatsoever this smells somewhat more like a problem with the site (or maybe with pdf.js).
Flags: needinfo?(cam)
So the NS_ERROR_FAILURE is coming from the .fonts getter on a document.  It's being called on a document with no presshell.

So there are a few issues here:

1)  Presumably it should be ok to do this on documents that are not rendered and have it not throw (needs a spec issue to define behavior?).

2)  GetFonts is not flushing, so it's possible to have this fail just because we haven't constructed the frame for the <iframe> yet.
Though adding a flush doesn't help.
Assignee: nobody → cam
Component: CSS Parsing and Computation → DOM: CSS Object Model
Flags: needinfo?(cam)
OS: Windows 7 → All
Hardware: x86_64 → All
I'm not really sure how much of a useful FontFaceSet we can return without a pres shell.  I guess we could still do the actual loading and set manipulation, though we'd need to move some stuff from nsPresContext to Document.  Until we work all that out, maybe we should make document.fonts return null in this case?  That should make the feature detection here think that we don't support it.  It's doing !!document.fonts, and that seems like a reasonable method of feature detection that we shouldn't mess with by throwing an exception.
Attached patch patchSplinter Review
Attachment #8601288 - Flags: review?(bugs)
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.
Comment on attachment 8601288 [details] [diff] [review]

So this is not what the spec says. .fonts should be non-null.
But then, "Any document, workers, or other context which can use fonts in some manner must implement the FontFaceSource interface." is a bit vague.
Can document.implementation.createHTMLDocument()
"use fonts"?

Chrome seems to return non-null always.
Attachment #8601288 - Flags: review?(bugs) → review-
It also doesn't say that you're allowed to throw an exception. :-)

The object returned by createHTMLDocument() will have to implement FontFaceSource, given you can't make a decision for individual objects about which interfaces they implement.

If you think it would be better to get the whole thing working in display:none iframes without this interim temporary fix, that's fine.  (I'll do that in bug 1161413.)
Perhaps we should disable .fonts until it works on all the documents?
(I don't know if .fonts is enabled on non-Nightly too)
It's currently only enabled on Nightly.  (It's #ifdef RELEASE_BUILD but hasn't merged to Aurora yet.)
I sent a mail to ask about document.fonts on documents not associated with a browsing context (such as those returned by createHTMLDocument):
Is this still a problem?  We've now shipped the font loading API.

Needinfo? to myself since :heycam is away.
Flags: needinfo?(dbaron)
Should this bug just be closed, or does something need to happen here?
Flags: needinfo?(dbaron) → needinfo?(cam)
Bug 1161413 fixed this.
Closed: 8 years ago
Depends on: 1161413
Flags: needinfo?(cam)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.