1539979 - Use type="content" for devtools toolbox frames

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

Nika asked if it was possible to load devtools toolbox UI in a content frame (not content process, but just type="content" on the browser element), to support Bug 1525427.

It definitely is, because that's what we do in about:devtools-toolbox. But we don't do it currently when hosting the toolbox in side, bottom, or window hosts.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1539979 - Proof of concept - Use type="content" for devtools toolbox frames

Comment 2

[Brian Grinstead [:bgrins]]

A few notes:

• There's some UI glitch with the panel for switching hosts after moving into window host. It's anchoring incorrectly into the middle of the toolbox.
• With type="content" you can't do this.win.parent from the toolbox, so there's a new getter added that does the voodoo necessary. It's likely we'd want to refactor how toolbox hosts feeds in a parent window (instead of just the content window). Doing so doesn't look as trivial as I'd hoped since we only instantiate the toolbox once and the window that we postMessage to / listen to changes when switching from a non-window host to a window host.
• Don't know how many tests this'll break. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71716e3170e80365fbfc7157534b684cfba8cbe9.
• We could change the xul iframes to xul browsers if we want. It seems to work OK with either.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #2)

Thanks a lot for looking into that!

A few notes:

• There's some UI glitch with the panel for switching hosts after moving into window host. It's anchoring incorrectly into the middle of the toolbox.

We are using the famous swapFrameLoader over here:
https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-host-manager.js#214-215
That may have some side effects with the new setup?
I already discussed with Nika about us using this ancient trick, and especially in the RDM.
But I got to understand that it would be the typical way to change the process of an iframe when navigating to an origin that has to run in another process. So hopefully, this will keep working and even be improved!

• With type="content" you can't do this.win.parent from the toolbox, so there's a new getter added that does the voodoo necessary.

I think that there is only a couple of callsites trying to get the parent window. I recall working a lot around this when adding the ToolboxHostManager.

It's likely we'd want to refactor how toolbox hosts feeds in a parent window (instead of just the content window). Doing so doesn't look as trivial as I'd hoped since we only instantiate the toolbox once and the window that we postMessage to / listen to changes when switching from a non-window host to a window host.

Note that this postMessage dance was introduced in order to plan running the toolbox OOP and/or in a content page.
If we ever run the toolbox OOP or with content privileges, we would no longer allow any direct Javascript calls.
So keep that in mind that we should avoid adding calls between chrome code and the toolbox.

• We could change the xul iframes to xul browsers if we want. It seems to work OK with either.

Would that change anything regarding bug 1525427. It isn't clear why devtools should use type=content?
What does it change given that we are loading a chrome URI and are still privileged and in parent process?
Were we the only usage of <iframe> without type=content in the chrome of Firefox?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

• We could change the xul iframes to xul browsers if we want. It seems to work OK with either.

Would that change anything regarding bug 1525427. It isn't clear why devtools should use type=content?
What does it change given that we are loading a chrome URI and are still privileged and in parent process?
Were we the only usage of <iframe> without type=content in the chrome of Firefox?

Fowarding this question to Nika.

Comment 5

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

bug 1525427 is blocked on this due to some assertion failures which are raised by the newly added code. When changing hosts, the toolbox performs a swapFrameLoaders call. BrowsingContext right now requires that a BrowsingContext's parent doesn't change during one of these swaps, which is normally OK for content frames due to content frames being part of separate BrowsingContext trees. Unfortunately devtools violates this with the switchHost() call, as a non type="content" frame has a parent, and performing a swapFrameLoaders call will cause the parent to change.

It doesn't make a difference what type of frame is being used, so long as the embedder is not in the same BrowsingContext tree when a swap is occurring. (this is why RDM doesn't trip this particular code).

Comment 6

[Neha Kochar [:neha]]

Confirmed with Sole that Julian is working on this, so updating the assignee here.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Use docShell.chromeEventHandler to catch contextmenu/keydown events

Depends on D25306

Comment 8

[Julian Descottes [:jdescottes]]

As discussed, the plan here will be to land a first version that will make DevTools toolbox frames use type="content" if the preference fission.rebuild_frameloaders_on_remoteness_change is set to true.

We will address the remaining breakage in follow up bugs. Bug 1541970 was already filed to take care of XUL panel positioning issues. We made some good progress on the overall task. The last try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1733cb3bb295877ad7b05aebcb1c8b370f33717 shows that we still have to fix two main issues:

• devtools webextensions are not working
• performance panel is broken

So far we have fixed:

• XUL panel positioning
• keyboard shortcut registration
• context menu issues
• other misc issues occurring during host swapping

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Add helper to create devtools iframes;r=bgrins

Shared helper to create DevTools frames for side and bottom hosts.
Will make it easier to swap to type="content" in a single spot.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Create toolbox iframe for Window host with helper;r=bgrins

Depends on D26316
Instead of using a hardcoded iframe in toolbox-window.xul, reuse the same helper as for the other hosts.
Will facilitate switching to type="content".

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Set type=content for DevTools toolbox frames when rebuild_frameloaders_on_remoteness_change is true;r=bgrins

Depends on D26317

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Initialize browser-loader only after domready in toolbox.js;r=bgrins

Depends on D26319

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Use toolbox window as telemetry event object;r=miker

Depends on D26320

@miker: We are moving devtools iframes to use type=content, and win.top will no longer be available.
We could use another getter to get the topmost chrome window, but reading the code here, I thought that maybe using the toolbox window would be better.
Today if you open several toolboxes in different tabs of the same window, the events for the various toolboxes will be mixed together, because win.top is the same for all the toolboxes that live in the same window.
It might be better to use an object which is unique to the toolbox?

Comment 14

[Julian Descottes [:jdescottes]]

try at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ffa0a209570fdbeb4d5091b17d81a452af809b

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Julian Descottes [:jdescottes] from comment #8)

• performance panel is broken

Does the new performance panel work (devtools.performance.new-panel-enabled=true)?

Comment 16

[Julian Descottes [:jdescottes]]

(In reply to Panos Astithas (he/him) [:past] (please ni?) from comment #15)

(In reply to Julian Descottes [:jdescottes] from comment #8)

• performance panel is broken

Does the new performance panel work (devtools.performance.new-panel-enabled=true)?

Yes the new perf panel works, and the symptoms with the old performance panel are UI related. All the diagrams and charts are blank. Knowing that the old performance panel is one of the few panels still in XUL, it's not too surprising. I will start investigating the issues today. I'll report here in case we hit a real blocker, I hope this can be solved without too much effort.

Comment 17

[Julian Descottes [:jdescottes]]

A fix for the performance panel issue is up for review in Bug 1532993.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2aceb1e2f211
Add helper to create devtools iframes;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/c5a67ce1a11f
Create toolbox iframe for Window host with helper;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/b2f79b98021d
Set type=content for DevTools toolbox frames when rebuild_frameloaders_on_remoteness_change is true;r=bgrins,nika
https://hg.mozilla.org/integration/autoland/rev/b14aadc29bf7
Initialize browser-loader only after domready in toolbox.js;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/20f11a17eace
Use toolbox window as telemetry event object;r=miker

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2aceb1e2f211
https://hg.mozilla.org/mozilla-central/rev/c5a67ce1a11f
https://hg.mozilla.org/mozilla-central/rev/b2f79b98021d
https://hg.mozilla.org/mozilla-central/rev/b14aadc29bf7
https://hg.mozilla.org/mozilla-central/rev/20f11a17eace

Comment 20

[Julian Descottes [:jdescottes]]

The main remaining issue are devtools webextensions. I haven't made much progress on this yet since I am new to this part of the codebase. Filed Bug 1543710 focused on this issue.

Comment 21

[Julian Descottes [:jdescottes]]

I want to make sure everyone is on the same page regarding this bug. Since fixing DevTools in frames with type="content" will take some time, a workaround will be used in order to unblock the Fission work (bug 1544419).

Once the remaining blocking DevTools bugs are fixed (Bug 1543710 and Bug 1543940), we will land an additional patch in this bug to enable type="content" for DevTools frames and close this bug.

Since we can't land the change behind a preference, I am closing Bug 1542261 immediately. This bug was supposed to flip said preference to true and is now irrelevant. To follow the progress about DevTools running in type="content", this bug remains the main bug to watch. More focused discussions about the remaining technical issues can be found in the blocking bugs.

Comment 22

[Neha Kochar [:neha]]

Workaround to unblock DOM landed in Fission M2, but the remaining work will land in M3, so moving this bug to M3.

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Use a frame with type=content for DevTools frames

This introduces a preference to load the DevTools toolbox
in a frame with type=content instead of type=chrome.
Having a preference will allow to keep the previous behavior in a
few intermittent tests, while we collect feedback on Nightly.

Comment 25

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Attach SwatchBasedEditor shortcuts to tooltip window

Depends on D40988

This is not explicitly needed in this queue but I noticed this issue
while working on eyedropper test issues.

The top window might change when switching from docked to window hosts
so we should rather attach shortcut events to the regular window.

Comment 26

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Fix HTML Tooltip test for content frames

Depends on D40989

I though I fixed this a few months ago, but there are still timing issues with the current test when using a frame with type=content.

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Fix browser_toolbox_window_title_changes for DevTools in content frame

Depends on D40990

Comment 28

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Fix browser_toolbox_toolbar_overflow.js when running in content frame

Depends on D40991

Comment 29

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Fix browser_inspector_search-filter_context-menu.js when running in content frame

Depends on D40992

Comment 30

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Fix eyedropper tests for DevTools in content frame

Depends on D40993

I don't fully understand the failures in this test on Linux.
The only way I could get it to pass for now was to force a window focus and wait.
Other OSes are fine, but without this, Linux is permafailing this.

I don't have a good linux env right now, so I would like to proceed with this and
fix it in a follow up.

Comment 31

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Add delay in various DevTools mochitests after switching hosts

Depends on D40994

Switching hosts (especially going from docked to window) seems more async than before.
I would like to work on fixing those in a follow up.

Comment 32

[Julian Descottes [:jdescottes]]

Attachment: Bug 1539979 - Force devtools to use a chrome frame in intermittent mochitests

Depends on D40995

I get a 10% failure rate on try for those two tests, but I haven't managed to get
a single failure locally so I couldn't identify the root cause yet. I would like
to fix them in a follow up.

Comment 33

[Julian Descottes [:jdescottes]]

Latest try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=987daeb27fcb22b64b2cf89d2ff0c320de02736c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6487a127ca4992dded31f1af88643c2ac6586bc2

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/375669bcc8c4
Use a frame with type=content for DevTools frames r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d6a8af7bffd0
Attach SwatchBasedEditor shortcuts to tooltip window r=gl
https://hg.mozilla.org/integration/autoland/rev/9671b91372a0
Fix HTML Tooltip test for content frames r=gl
https://hg.mozilla.org/integration/autoland/rev/3ef8e8b15a55
Fix browser_toolbox_window_title_changes for DevTools in content frame r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ecf444406485
Fix browser_toolbox_toolbar_overflow.js when running in content frame r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/82d43d3d5644
Fix browser_inspector_search-filter_context-menu.js when running in content frame r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e7d974a6ba03
Fix eyedropper tests for DevTools in content frame r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a854a320ef6a
Add delay in various DevTools mochitests after switching hosts r=ochameau
https://hg.mozilla.org/integration/autoland/rev/39e19afc797d
Force devtools to use a chrome frame in intermittent mochitests r=ochameau

Comment 35

[Julian Descottes [:jdescottes]]

For reference my last try push for this stack is https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5738afec6482bd5abb60ea198458b8e04f48d34

Comment 36

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/375669bcc8c4
https://hg.mozilla.org/mozilla-central/rev/d6a8af7bffd0
https://hg.mozilla.org/mozilla-central/rev/9671b91372a0
https://hg.mozilla.org/mozilla-central/rev/3ef8e8b15a55
https://hg.mozilla.org/mozilla-central/rev/ecf444406485
https://hg.mozilla.org/mozilla-central/rev/82d43d3d5644
https://hg.mozilla.org/mozilla-central/rev/e7d974a6ba03
https://hg.mozilla.org/mozilla-central/rev/a854a320ef6a
https://hg.mozilla.org/mozilla-central/rev/39e19afc797d