1153230 - Github failed "pdf viewing" on Firefox desktop nightly

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

githab now support pdf viewing by using pdf.js
    https://github.com/blog/1974-pdf-viewing

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.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaManager_FirefoxOS_2_2.pdf

Comment 1

[Sotaro Ikeda [:sotaro]]

The problem happened also when e10s was disabled.

Comment 2

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aca9840cf06f&tochange=8476f8a5e00b

Triggered by: Bug 1088437

setting layout.css.font-loading-api.enabled = false helps

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Though adding a flush doesn't help.

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 9

[Cameron McCormack (:heycam)]

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 10

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

Comment on attachment 8601288 [details] [diff] [review]
patch

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.

Comment 11

[Cameron McCormack (:heycam)]

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

Comment 12

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

Perhaps we should disable .fonts until it works on all the documents?
(I don't know if .fonts is enabled on non-Nightly too)

Comment 13

[Cameron McCormack (:heycam)]

It's currently only enabled on Nightly.  (It's #ifdef RELEASE_BUILD but hasn't merged to Aurora yet.)

Comment 14

[Cameron McCormack (:heycam)]

I sent a mail to ask about document.fonts on documents not associated with a browsing context (such as those returned by createHTMLDocument): https://lists.w3.org/Archives/Public/www-style/2015May/0091.html

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this still a problem?  We've now shipped the font loading API.

Needinfo? to myself since :heycam is away.

Comment 16

[Alice0775 White]

WFM on 45.0RC and later on Windows7.

Fixed range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e7d5ce4c80f&tochange=b6379bec3c17

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should this bug just be closed, or does something need to happen here?

Comment 18

[Cameron McCormack (:heycam)]

Bug 1161413 fixed this.