1606135 - make BackgroundPageThumbs fission compatible

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P3)

Description

[alexander :surkov (:asurkov)]

similar to bug bug 1576911, the logic should be switched to drawSnapshot API which is fission compatible (see bug 1475139). Beyond this, it seems we could do a number cleanup/code improvements here. Will file those as dependent bugs.

Comment 1

[:Gijs (he/him)]

metadata for the file says bugs are tracked in New Tab Page so let's move this there.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 3

[Gavin Suntop [:gvn] (they/them)]

Can you point us to some documentation around what the best practice is for remediation here?

Comment 4

[:Gijs (he/him)]

(In reply to Gavin Suntop [:gvn] (he/they) from comment #3)

Can you point us to some documentation around what the best practice is for remediation here?

BackgroundPageThumbs uses a frame script ( backgroundPageThumbsContent.js ) right now. This is not compatible with fission. comment #0 suggests you could use the drawSnapshot API instead of having the frame script take a screenshot and then passing that back to the parent.

If more JS still needs to run in the content process for the background page thumbs code to work (which I'm not sure about) it would need to be converted to using a JSWindowActor. See e.g. https://phabricator.services.mozilla.com/D50584 for an example, https://firefox-source-docs.mozilla.org/dom/Fission.html#jswindowactor and https://firefox-source-docs.mozilla.org/dom/Fission.html#how-to-port-from-message-manager-and-framescripts-to-jswindowactors etc. for docs.

One issue I suspect may be problematic here is that AIUI there is currently no way to limit where we run JSWindowActors, except by URI. But in this case, we really want the actor to only run in the browser/docshell created for the background page thumbs. It'd be worth liaising directly with the fission team to figure that part out, if you end up needing to run JS.

Does that help?

Comment 5

[Gavin Suntop [:gvn] (they/them)]

Yes, thank you!

Comment 6

[Neil Deakin]

Attachment: Bug 1606135, switch background page thumbs service to use drawSnapshot and an actor instead of a framescript, r=adw

Comment 7

[Chris Peterson [:cpeterson]]

Tracking for Fission M6c Nightly milestone.

Comment 8

[Neil Deakin]

Now that windowless browser supports fission, I now have the background page thumbnailer creating new processes for each domain that it loads. However, this requires passing 'Ci.nsIWebBrowserChrome.CHROME_REMOTE_WINDOW | Ci.nsIWebBrowserChrome.CHROME_FISSION_WINDOW' to AppShell::CreateWindowlessBrowser when fission is enabled. I know we want to avoid using the fission.autostart preference, but it seems like this is a case where it can't be avoided. Does this sound right?

Comment 9

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

Yes, I believe background processes which need to create windows unrelated to any other existing window and need to pick whether those windows have Fission enabled will need to check the fission.autostart pref directly still.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f04d5ddcee0
switch background page thumbs service to use drawSnapshot and an actor instead of a framescript, r=adw

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1f04d5ddcee0