1571419 - Make firefox --screenshot Fission compatible

Status: RESOLVED FIXED

(Firefox :: Headless, task, P3)

Description

[Andrew Swan [:aswan]]

Firefox --screenshot uses canvas and drawWindow() to take a screenshot. This will stop working when cross-origin iframes move into remote processes.
Fortunately, work is already scheduled (bug 1499845) to address this for devtools which has the same issue. When that bug is completed, browser/components/shell/HeadlessShell.jsm should be updated to use it (which will have the additional benefit of simplifying HeadlessShell.jsm and reducing duplicate code)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that I also do the same right now for Marionette (bug 1559592).

Comment 2

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 3

[Chris Peterson [:cpeterson]]

Brendan, who currently uses headless Firefox --screenshot? Should this bug block enabling Fission in Nightly (Fission M6 milestone)?

Headless screenshots are currently broken with Fission, but the comments above suggest fixing it should be straightforward.

Comment 4

[Brendan Dahl [:bdahl]]

(In reply to Chris Peterson [:cpeterson] from comment #3)

Brendan, who currently uses headless Firefox --screenshot? Should this bug block enabling Fission in Nightly (Fission M6 milestone)?

Headless screenshots are currently broken with Fission, but the comments above suggest fixing it should be straightforward.

It's mainly a developer feature. Unfortunately, I don't think we have ever collected telemetry on how often it's used, but when it's broken people have complained. I'll see if I can devote a little time to it this week to port it over to the new code. If anyone else wants to take it though feel free.

Comment 5

[Neha Kochar [:neha]]

Brendan, the API is already ready and Marionette is fixed too. How soon can this be fixed?

Comment 6

[Brendan Dahl [:bdahl]]

I'm not going to have any bandwidth to work on this in the near future.

Henrik, since you already fixed marionette, do you think you could convert headless too?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Brendan Dahl [:bdahl] from comment #6)

Henrik, since you already fixed marionette, do you think you could convert headless too?

Sorry, but I won't have time to make changes to that Firefox feature. My focus is on Fission work for Marionette right now.

Nevertheless the changes should be easy to do. Here how Marionette calls the API:
https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/testing/marionette/capture.js#135-141

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As further information the JSWindowActor pair might not be needed for the headless changes. The screenshots can be taken from the parent process but just specifying the correct browsing context.

Comment 9

[Chris Peterson [:cpeterson]]

moving up to M7 because people might be using --screenshot for testing.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

It's been a few months, and we're nearing the timeline for M7. Is there any chance you would be able to get to looking into this issue soon :bdahl?

Comment 11

[Brendan Dahl [:bdahl]]

I'll take a stab at it this week or early next week.

Comment 12

[Brendan Dahl [:bdahl]]

Attachment: Bug 1571419 - Make --screenshot fission compatible. r=kmag

Move capturing the window into the parent process to use the new
drawSnapshot API.

Comment 13

[Brendan Dahl [:bdahl]]

Attachment: Bug 1571419 - Add test for cross origin iframe screenshot. r=kmag

Depends on D104734

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c018b593000
Make --screenshot fission compatible. r=kmag
https://hg.mozilla.org/integration/autoland/rev/f96850b1cb38
Add test for cross origin iframe screenshot. r=kmag

Comment 15

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failure on browser_headless_screenshot.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/ea3cda33fbd843b344bc2469f54343f3a736fae6
Log link: https://treeherder.mozilla.org/logviewer?job_id=330446741&repo=autoland&lineNumber=3069

Comment 16

[Brendan Dahl [:bdahl]]

Test is failing with This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.
Kris,
Are you fine with me using requestLongerTimeout? Not sure I see a way to make this test faster as it starts up a whole new Firefox.

Comment 17

[Kris Maglione [:kmag]]

(In reply to Brendan Dahl [:bdahl] (84 regression triage) from comment #16)

Test is failing with This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.
Kris,
Are you fine with me using requestLongerTimeout? Not sure I see a way to make this test faster as it starts up a whole new Firefox.

I'd be OK with that, but it would probably be better to split it into multiple tests, given how easy it would be. We could just extract the helper functions at the top of the file either into head.js or a separate script that we load with the subscript loader, and then move the test*(...) calls into a few separate groups in separate test files. That way, if one of them starts failing, it becomes easier to debug, because the test doesn't take as long to run.

That said, if you do decide to just increase the timeout, we should probably only do it on TSan, since that's the only configuration that seems to actually be failing.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06c711c8b646
Make --screenshot fission compatible. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6703954605a4
Add test for cross origin iframe screenshot. r=kmag

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/06c711c8b646
https://hg.mozilla.org/mozilla-central/rev/6703954605a4