1921471 - Allow non-parent process sidebar panes in the sidebar

Status: NEW

(Firefox :: Sidebar, task)

Description

[:Gijs (he/him)]

The sidebar loads a bunch of chrome URLs. This hasn't changed since the pre-e10s/fission days. This is annoying when for more modern code we'd like to use non-parent process frames. Webextensions already work around this by having a single parent-process doc that then has its own nested <browser> element that then shows a content-process webextension document... The double nesting is awkward and seems like something we would ideally get rid of.

There are a bunch of assumptions baked into the code at this point but now that process switching has largely moved into DOM code it doesn't look like fixing this is too awful a job. However, I'm curious about any fundamental problems we might run into when doing something like this; Nika, anything that comes to mind? (e.g. about the number of type=content child frames we expect for browser.xhtml or other assumptions we make)

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1921471 - Use content process to display sidebar contents when appropriate. r?#shopping-reviewers!,mossop!

Comment 2

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

Left some comments on the WIP patch you've posted with some thoughts.

The main thing I expect to break is around the sidebar code now being loaded as a content frame rather than a chrome frame. You mention that previously webextensions have loaded with a nested remote browser within them, and that is not allowed by-default in type=content frames, even within the parent process, so you may need to add more exceptions to https://searchfox.org/mozilla-central/rev/9fa446ad77af13847a7da250135fc58b1a1bd5b9/dom/base/nsFrameLoader.cpp#2601-2613.

Any methods which were handled by directly interacting with the frame will also break, as the frame could be in a separate process, so it seems likely you'll want to add a new messagemanagergroup for the sidebar browser, and move a number of these functions to instead be done over JSWindowActors.

Comment 3

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

Any methods which were handled by directly interacting with the frame will also break, as the frame could be in a separate process, so it seems likely you'll want to add a new messagemanagergroup for the sidebar browser, and move a number of these functions to instead be done over JSWindowActors.

Mm, well, the contentWindow and contentDocument invocations seem to still work when the contents are in the same process, so as long as we're not changing which process we use (ie which URL we use) to load some of this at the same time, we could tackle this separately. It doesn't look like there's too much logic that cares about the contents, apart from the SidebarFocused thing (which I don't think we need) and the load handling.

Comment 4

[Fred Chasen [:fchasen]]

Attachment: WIP: Bug 1921471 - use content process to display sidebar contents when appropriate. (FCHASEN TEST)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04f7f20bf82e
Use content process to display sidebar contents when appropriate. r=places-reviewers,sidebar-reviewers,mossop,firefox-desktop-core-reviewers ,jsudiaman

Comment 6

[agoloman]

Backed out for causing bc failures @browser_syncedtabs_sidebar.js.

• Backout link
• Push with failures
• Failure Log
  ————————————————————————————————————
• Push with failures bc failures @browser_unavailable_product.js
• Failure Log

Comment 7

[Sarah Clements [:sclements]]

Fred, can you hold off on landing this until the 135 Nightly begins on Monday? I'd be good to minimize anything that could cause regressions in 134, since we have a sidebar/vertical tabs experiment continuing in 134.

Comment 8

[Fred Chasen [:fchasen]]

Thanks Sarah, sorry for rushing to get this in - definitely better to hold off.

Was surprised to see this failing as both tests worked for me when run separately but failed when run back to back. However, I just needed to make sure to call ShoppingSidebar.hide() at the end of the remote test.

Going to make sure to always close sidebars I open in tests now, as browser_syncedtabs_sidebar.js will fail to stub the panel if the view tabs sidebar panel has already been opened in a previous test.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4369fa6e979
Use content process to display sidebar contents when appropriate. r=places-reviewers,sidebar-reviewers,mossop,firefox-desktop-core-reviewers ,jsudiaman

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_sidebar_remote.js & browser_unavailable_product.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/a58452cfb675e2b43de0b0dc8ee60b8536300679

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/sidebar/tests/browser/browser_sidebar_remote.js

Failure log -> TEST-UNEXPECTED-TIMEOUT | browser/components/shopping/tests/browser/browser_unavailable_product.js (finished) | application timed out after 370 seconds with no output

Comment 11

[Fred Chasen [:fchasen]]

I've abandoned the patch to update the sidebar to handle remote content, as there are just too many extension sidebar relate tests were failing to get that patch landed without the updating how extensions in the sidebar work.

I still think switching all the sidebar over to remote content is the right approach in the long term. That could be using a single type=content browser or creating a new browser element to load a panel and removing the current browser on hide. To me the later seems consistent with other browser element uses, and may be needed to prevent leaks (or at least to prevent tests from looking like leaks).

However I don't think those are possible to implement without updating how sidebar extensions are loaded at the same time to make it all work together. I'll put together what I think needs to be updated from what I have tried so far so we can try and figure out the size of that project.