Followup to 1456485, make Screenshots work on PDFs

VERIFIED FIXED in Firefox -esr60

Status

defect
P1
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: zombie, Assigned: zombie)

Tracking

62 Branch
mozilla62
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr6061+ verified, firefox60 wontfix, firefox61+ verified, firefox62+ verified)

Details

Attachments

(3 attachments)

There are a few issues, the main being that Screenshots is an embedded WE, which doesn't get passed proper addonData flags to enable privileged status and mozillaAddons permission.
Assignee

Updated

Last year
Assignee: nobody → tomica
Blocks: 1456485
Status: NEW → ASSIGNED
Priority: -- → P1
Do we need to add a similar exception to manifest.json for reader mode too?
Assignee

Updated

Last year
Attachment #8982843 - Flags: review?(aswan)
Attachment #8982844 - Flags: review?(jhirsch)
Attachment #8982844 - Flags: review?(aswan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8982919 - Flags: review?(aswan)
Assignee

Comment 9

Last year
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Do we need to add a similar exception to manifest.json for reader mode too?

Added.
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

I can confirm that the Try builds are working perfectly with both the PDF viewer and Reader Mode :)
Attachment #8982919 - Flags: feedback+

Comment 11

Last year
mozreview-review
Comment on attachment 8982843 [details]
Bug 1466349 part 1 - Enable passing addonData flags to embedded WE,

https://reviewboard.mozilla.org/r/248706/#review255098

::: commit-message-8c926:1
(Diff revision 2)
> +Bug 1466349 part 1 - Enable passing addonData flags to embedded WE

This patch also fixes the creation of the matcher for origin permissions in child processes, please mention that in the commit message.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:136
(Diff revision 2)
> +   *   Additional data to pass to the Extension constructor.
>     *
>     * @returns {Promise<LegacyContextAPI>} A promise which resolve to the API exposed to the
>     *   legacy context.
>     */
> -  startup(reason) {
> +  startup(reason, addonData = null) {

nit: could just make the default here be {} and avoid the || below
Attachment #8982843 - Flags: review?(aswan) → review+

Comment 12

Last year
mozreview-review
Comment on attachment 8982844 [details]
Bug 1466349 part 2 - Pass addonData to embedded Screenshots WE, add permissions

https://reviewboard.mozilla.org/r/248708/#review255102
Attachment #8982844 - Flags: review?(aswan) → review+

Comment 13

Last year
mozreview-review
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

https://reviewboard.mozilla.org/r/248744/#review255106

::: commit-message-64fd3:1
(Diff revision 2)
> +Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

Please add an explanation for why we want to do such a think.
Attachment #8982919 - Flags: review?(aswan) → review+
Assignee

Updated

Last year
Attachment #8982844 - Flags: review?(jhirsch) → review?(ianb)

Comment 14

Last year
mozreview-review
Comment on attachment 8982844 [details]
Bug 1466349 part 2 - Pass addonData to embedded Screenshots WE, add permissions

https://reviewboard.mozilla.org/r/248708/#review255518

I've created a Screenshots bug for including this patch: https://github.com/mozilla-services/screenshots/issues/4528
Attachment #8982844 - Flags: review?(ianb) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

Last year
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b84f57938f8b
part 1 - Enable passing addonData flags to embedded WE, r=aswan
https://hg.mozilla.org/integration/autoland/rev/e04c7ccfd50e
part 2 - Pass addonData to embedded Screenshots WE, add permissions r=aswan,ianbicking
https://hg.mozilla.org/integration/autoland/rev/0bb149ce1d1a
part 3 - Ignore ignorePath for privileged about: MatchPatterns r=aswan
Assignee

Comment 19

Last year
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

> Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1436482.

> [User impact if declined]:
Firefox Screenshots feature doesn't work on PDF pages.

> [Is this code covered by automated tests?]:
Partially.  Added tests to cover underlying WE support, not sure if the Screenshots team wants to add full integration testing.

> [Has the fix been verified in Nightly?]:
No, only manual testing done from the Try build by Ryan^.

> [Needs manual test from QE? If yes, steps to reproduce]: 
Yes, see bug 1456485.

> [List of other uplifts needed for the feature/fix]:
Bug 1456485.

> [Is the change risky?]:
Somewhat.

> [Why is the change risky/not risky?]:
It only enables privileged (mozilla-signed) extensions to access protected pages.

> [String changes made/needed]:
None.
Attachment #8982919 - Flags: approval-mozilla-beta?
Assignee

Comment 20

Last year
(In reply to Tomislav Jovanovic :zombie from comment #19)
> > Approval Request Comment
> > [List of other uplifts needed for the feature/fix]:
> Bug 1456485.

Oops, forgot to note bug 1465544 as well.
Flags: qe-verify+
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

While I'd still like the SV folks to give this a more detailed look, I can at least confirm that everything appears to be working as expected with the latest Nightly build including these patches.

Approved for 61.0b12.
Attachment #8982919 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have verified this issue on the latest Nightly (62.0a1 Build ID: 20180606083531) build and the issue is no longer reproducible. Firefox Screenshots works now on PDF pages.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.
Comment on attachment 8982919 [details]
Bug 1466349 part 3 - Ignore ignorePath for privileged about: MatchPatterns

Verified on Beta, taking for ESR 60.1 as well.
Attachment #8982919 - Flags: approval-mozilla-esr60+
I have verified this issue on the latest Beta (61.0b14, Build ID: 20180614135649) and the issue is no longer reproducible.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.

The issue is still reproducible on Firefox ESR 60.0 because of bug 1467924 that should also be uplifted in ESR 60 (see bug 1467924 comment 10).

Updated

Last year
Product: Toolkit → WebExtensions
I have verified the issue on latest ESR (60.1.0, Build ID 20180621121604) and the issue is no longer reproducible. Tested on Windows 10 x64, Windows 7 x64, Mac 10.13 and Arch Linux 4.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.