Github failed "pdf viewing" on Firefox desktop nightly

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sotaro, Assigned: heycam)

Tracking

({qawanted, regression})

Trunk
qawanted, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Keywords: qawanted, regressionwindow-wanted
(Reporter)

Comment 1

4 years ago
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

Comment 2

4 years ago
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
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.

Updated

4 years ago
Keywords: regressionwindow-wanted → regression
(Assignee)

Updated

4 years ago
Assignee: nobody → cam
Component: CSS Parsing and Computation → DOM: CSS Object Model
Flags: needinfo?(cam)
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
Created attachment 8601288 [details] [diff] [review]
patch
Attachment #8601288 - Flags: review?(bugs)
(Assignee)

Comment 9

4 years ago
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]
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.
Attachment #8601288 - Flags: review?(bugs) → review-
(Assignee)

Comment 11

4 years ago
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)
(Assignee)

Comment 13

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

Comment 14

4 years ago
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
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)
(Assignee)

Comment 18

3 years ago
Bug 1161413 fixed this.
Status: NEW → RESOLVED
Last Resolved: 3 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.