Closed Bug 1571419 Opened 5 years ago Closed 3 years ago

Make firefox --screenshot Fission compatible

Categories

(Firefox :: Headless, task, P3)

task

Tracking

()

RESOLVED FIXED
88 Branch
Fission Milestone M7
Tracking Status
firefox88 --- fixed

People

(Reporter: aswan, Assigned: bdahl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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)

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

Priority: -- → P3

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

Fission Milestone: --- → ?

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.

Fission Milestone: ? → M6
Flags: needinfo?(bdahl)
See Also: → 1559592

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

Flags: needinfo?(bdahl)
Fission Milestone: M6 → M6c

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

Fission Milestone: M6c → MVP
Flags: needinfo?(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?

Flags: needinfo?(bdahl) → needinfo?(hskupin)

(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

Flags: needinfo?(hskupin)

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.

See Also: → 1678483

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

Severity: normal → S3
Fission Milestone: MVP → M7

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?

Flags: needinfo?(bdahl)

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

Assignee: nobody → bdahl
Flags: needinfo?(bdahl)

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

Pushed by bdahl@mozilla.com:
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

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.

Flags: needinfo?(bdahl) → needinfo?(kmaglione+bmo)

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

Flags: needinfo?(kmaglione+bmo)
Pushed by bdahl@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Regressions: 1701449
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: