Closed Bug 1539979 Opened 9 months ago Closed 4 months ago

Use type="content" for devtools toolbox frames

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(Fission Milestone:M4, firefox70 fixed)

RESOLVED FIXED
Firefox 70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: bgrins, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission)

Attachments

(14 files, 2 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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.

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

Blocks: dt-fission

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

Flags: needinfo?(nika)

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

Flags: needinfo?(nika)
Priority: -- → P3

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

Assignee: nobody → jdescottes
Priority: P3 → P2
Fission Milestone: --- → M2
Depends on: 1541970

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
Status: NEW → ASSIGNED
Summary: Make devtools use type="content" on frame elements that load toolbox.xul → Use type="content" for devtools toolbox frames if rebuild_frameloaders_on_remoteness_change=true
Blocks: 1542261

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

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

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?

(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)?

Keywords: leave-open

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

Depends on: 1532993

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

Pushed by jdescottes@mozilla.com:
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
Attachment #9054340 - Attachment is obsolete: true
Depends on: 1543710

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.

Depends on: 1543940
Attachment #9055500 - Attachment is obsolete: true
See Also: → 1544419

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.

Summary: Use type="content" for devtools toolbox frames if rebuild_frameloaders_on_remoteness_change=true → Use type="content" for devtools toolbox frames
No longer blocks: 1542261
Depends on: 1544709
Depends on: 1544749
Depends on: 1544752

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

Fission Milestone: M2 → M3
Blocks: 1342297
Duplicate of this bug: 1342297
Fission Milestone: M3 → M4
Depends on: 1566870
Blocks: 1571421
See Also: → 1571644

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.

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.

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.

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.

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.

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.

Keywords: leave-open
Priority: P2 → P1
Whiteboard: dt-fission
Regressions: 1571483
Regressions: 1572867
Pushed by jdescottes@mozilla.com:
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
Regressions: 1575766
Regressions: 1577881
Blocks: 1578979
Regressions: 1578373
Depends on: 1583444
See Also: → 1589617
You need to log in before you can comment on or make changes to this bug.