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

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: print preview simplified.png

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

Comment 1

[Raluca Popovici, Desktop QA]

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.

Comment 2

[Rob Wu [:robwu]]

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

Comment 3

[Raluca Popovici, Desktop QA]

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

Comment 4

[Rob Wu [:robwu]]

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

Comment 5

[Raluca Popovici, Desktop QA]

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.

Comment 6

[Julien Cristau [:jcristau]]

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

Comment 7

[:Gijs (he/him)]

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

Comment 8

[:Gijs (he/him)]

Attachment: 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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6d9b8a2a5bde

Comment 11

[:Gijs (he/him)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a046814f83aa

Comment 14

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/a4ae77fad8e9

Comment 17

[Catalin Sasca, Desktop Test Engineering [:csasca]]

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