Make firefox --screenshot Fission compatible
Categories
(Firefox :: Headless, task, P3)
Tracking
()
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)
Comment 1•5 years ago
|
||
Please note that I also do the same right now for Marionette (bug 1559592).
Assignee | ||
Updated•5 years ago
|
Comment 2•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
(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.
Assignee | ||
Updated•4 years ago
|
Comment 5•4 years ago
|
||
Brendan, the API is already ready and Marionette is fixed too. How soon can this be fixed?
Assignee | ||
Comment 6•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
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.
Updated•4 years ago
|
Comment 9•4 years ago
|
||
moving up to M7 because people might be using --screenshot for testing.
Comment 10•4 years ago
|
||
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?
Assignee | ||
Comment 11•4 years ago
|
||
I'll take a stab at it this week or early next week.
Assignee | ||
Comment 12•4 years ago
|
||
Move capturing the window into the parent process to use the new
drawSnapshot API.
Assignee | ||
Comment 13•4 years ago
|
||
Depends on D104734
Comment 14•4 years ago
|
||
Comment 15•4 years ago
|
||
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
Assignee | ||
Comment 16•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
Comment 19•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06c711c8b646
https://hg.mozilla.org/mozilla-central/rev/6703954605a4
Updated•4 years ago
|
Updated•4 years ago
|
Description
•