Closed Bug 1720877 Opened 3 months ago Closed 2 months ago

Simplified Format fails to load the article (PDF) in print preview

Categories

(Toolkit :: Reader Mode, defect)

defect

Tracking

()

VERIFIED FIXED
93 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- verified
firefox90 --- unaffected
firefox91 --- wontfix
firefox92 --- verified
firefox93 --- verified

People

(Reporter: csasca, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Affected versions

  • Firefox 91.0b3
  • Firefox 92.0a1

Affected platforms

  • macOS 10.14
  • Windows 10 & 8.1
  • Ubuntu 18.04

Steps to reproduce

  1. Launch Firefox
  2. Access a PDF sample which has simplified mode option
  3. Click on Print and select More Settings in Print preview
  4. Select Format from "Original" to "Simplified"

Expected result

  • The page is converted in simplified mode

Actual result

  • "Failed to load article from page" message is shown in Print Preview

Regression range

  • Will see for a regression

Additional notes

  • The issue can be seen in the attachment
Has Regression Range: --- → no
Has STR: --- → yes
QA Whiteboard: [qa-regression-triage]

I've managed to reproduce the issue on Windows 10x64, and then investigated for the change that made this behavior possible:
2021-07-20T09:08:08.651000: DEBUG : Found commit message:
Bug 1713628 - Treat notBefore in the future of signed XPI files as valid + tests r=keeler

This patch also includes unit tests for this + previous untested cases
(bug 1713628, bug 1267318 and bug 1548973).

The tool to generate the test cases (zip files) has also been updated
because it has been broken by changes from bug 1699294.

Differential Revision: https://phabricator.services.mozilla.com/D119802

Confirmed and reg window provided.

Has Regression Range: no → yes
QA Whiteboard: [qa-regression-triage]
Regressed by: 1713628

The reported regression range is incorrect.

The feature from the "Simplified" checkbox was recently introduced in bug 1666247. I downloaded the first build with that patch from https://archive.mozilla.org/pub/firefox/nightly/2021/07/2021-07-08-09-18-56-mozilla-central/ (referenced at https://hg.mozilla.org/mozilla-central/rev/8df8fb3d2e0e) and can still reproduce the issue.

The checkbox is shown when isReader is true. The reader button does appear in the location bar of the PDF file when I press the mouse button anywhere in the document (merely focusing it?), but upon clicking on that button the following error appears:

TypeError: can't access property "catch", ReaderMode.parseDocument(...) is null AboutReaderChild.jsm:50:45

After refreshing the page, the reader view button doesn't appear (and neither does the Simplified checkbox).

Keywords: regression
Regressed by: 1666247
No longer regressed by: 1713628

I notice that every time I run mozregression GUI I have different results:
Regression 2: Bug 1695195 - Set SandboxReporter profiler thread name r=jld

Regression 3: Bug 1719727 - Change "Personalize" on New Tab to gear icon. r=prathiksha,fluent-reviewers,Gijs,thecount

Regression 4: Bug 1720595 - Don't install dump_syms during mach bootstrap. r=firefox-build-system-reviewers,nalexander
Bug 1719229 - broke the install by accident, but configure takes care of
it by default since bug 1716911, bug 1716912 or bug 1717585 depending on
the platform, so we can just stop installing it from mach bootstrap.

Regression 5: Bug 1720696: Only fetch mMsaa from non-null AccessibleWraps r=eeejay

The inconsistent regression output is surprising.

What do you use to decide whether the bug was triggered?

The ideally observed behavior is that a print preview is shown when Simplified mode is used.
In comment 2 I noted that the bug either happened (as reported), or the Simplified checkbox didn't appear (after refreshing the page, in relation with the Reader mode button in the location bar).

I used mozregresssion GUI to find tthe regression and I introduce the following data:
Last known good build: 14-07-2021 (build without Simplified mode)
First known bad build : 15-07-2021(build with Simplified mode)
I observed that your bug is related to this one but mozregression didn't get anything of it on my runs.
Thank you for your help, Rob Wu.

Gijs should this be reassigned to reader mode (see comment 2, there's a TypeError when toggling reader mode on that pdf)?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Julien Cristau [:jcristau] from comment #6)

Gijs should this be reassigned to reader mode (see comment 2, there's a TypeError when toggling reader mode on that pdf)?

Reader mode doesn't work on PDFs and print preview shouldn't be offering it. I don't know why it does, and that's the first thing that should be worked out.

https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/toolkit/components/reader/ReaderMode.jsm#247-257

    if (
      !Readerable.shouldCheckUri(doc.documentURIObject) ||
      !Readerable.shouldCheckUri(doc.baseURIObject, true)
    ) {
      this.log("Reader mode disabled for URI");
      return null;
    }

is the implementation of the method call that leads to a type error. The call to _readerParse is to an async function, whereas parseDocument is not async, so the return value would always be a promise (it may be a rejected one, but not a non-promise). The only way that the type error happens is if the initial condition quoted above passes and parseDocument thus directly returns null - I'm guessing that it gets the PDF.js resource URI. Looks like that's because reader mode checks both the document and base URI, and isProbablyReaderable only checks the document URI.

Component: Print Preview → Reader Mode
Flags: needinfo?(gijskruitbosch+bugs)
Product: Core → Toolkit
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d9b8a2a5bde
ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r=robwu
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch

Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu

Beta/Release Uplift Approval Request

  • User impact if declined: Broken print preview and/or reader mode behaviour
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): 1-line JS change to add a condition that we check later anyway; checking it earlier means we don't end up with broken UI.
  • String changes made/needed: Nope
Attachment #9236816 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu

Approved for 92.0b6.

Attachment #9236816 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Verified that the simplified option is no longer available in pdfs. Tests were performed on Windows 10, macOS 10.15.7 and Ubuntu 20.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu

Approved for 91.1esr also.

Attachment #9236816 - Flags: approval-mozilla-esr91+

Verified the fix on 91.1esr as well. Tests were performed on Windows 7, macOS 10.15.7 and Ubuntu 20.

You need to log in before you can comment on or make changes to this bug.