STRs:

• set content="type" for devtools frames (in toolbox-hosts::createDevToolsFrame() add frame.setAttribute("type", "content");)
• try to run any webextension tests, eg browser_ext_devtools_inspectedWindow.js

ER: test should pass
AR: test fails. DevTools webextension will fail to load, logging errors related to window.messageManager being null in https://searchfox.org/mozilla-central/source/browser/base/content/webext-panels.js

Message Manager issues seem to be fixed with this patch, but tests now fail because browser.devtools is undefined. Looking for some help/pointers to know where I should look to fix this.

Luca, I'm trying to investigate failures around devtools webextensions that occur when devtools are in a "content" frame instead of a "chrome" frame.

I attached my current patch, but I'm stuck on tests failing with TypeError: browser.devtools is undefined. Can you give me some pointers to know where I can investigate the root cause for this? Thanks!

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

Luca, I'm trying to investigate failures around devtools webextensions that occur when devtools are in a "content" frame instead of a "chrome" frame.

I attached my current patch, but I'm stuck on tests failing with TypeError: browser.devtools is undefined. Can you give me some pointers to know where I can investigate the root cause for this? Thanks!

As briefly described in our quick chat over vidyo, I took a quick look and gave a try to the attached patch and here are some details from this initial investigation:

Some of the changes in the patch are currently breaking the loading of also other extension pages, like the background page.
In particular the changes applied to ExtensionCommon.jsm, ExtensionParent.jsm and MessageManagerProxy.jsm (and I think that these change should not be necessary in the final patch).

Then I tried to revert all the changes in the attached patch but I kept the frame.setAttribute("type", "content"); added to toolbox-hosts.js and temporarily commented out window.messageManager.addMessageListener("contextmenu", openContextMenu); and
window.messageManager.removeMessageListener("contextmenu", openContextMenu); from webext-panels.js, because I wanted to see what was actually breaking the loading of just the "extension devtools panel", while keeping everything else to still work as expected.

With this test I noticed that the XUL browser custom element is raising a exception when webext-panels.js creates the remote XUL browser to run the "devtools panel extension page" in the extension process and adds it to the DOM tree, and at that point the XUL browser custom element throws a NS_ERROR_UNEXPECTED error while calling ssm.getLoadContextCodebasePrincipal(aboutBlank, this.loadContext); here:

• https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/toolkit/content/widgets/browser-custom-element.js#1054

I added some additional logging right above that line to compare what this.frameLoader is with and without the "frame.setAttribute("type", "content"); added to toolbox-hosts.js" and see why ssm.getLoadContextCodebasePrincipal may be throwing, and I noticed that the frameLoader.loadContext seems to be null with that change, and it is not null without it.

Here are some additional details about the differences that I notice on frameLoader value for the "devtools_panel extension page" XUL browser, with and without the change in toolbox-hosts.js:

• browser.frameLoader.browserContext:

  • with change in toolbox-hosts.js: null
  • without: CanonicalBrowserContext { ..., currentRemoteType: "extension", ...}

• browser.frameLoader.messageManager:

  • with change in toolbox-hosts.js: ChromeMessageSender { processMessageManager: null, remoteType: "" }
  • without: ChromeMessageSender { processMessageManager: ProcessMessageManager, remoteType: "extension" }

• browser.frameLoader.tabParent:

  • with change in toolbox-hosts.js: null
  • without: XPCWrappedNative_NoHelper { ... }

My guess is that the actual underlying issue is likely related to these differences in the XUL browser element frameLoader, and I think that it would be helpful to ask :bgrins opinion on these differences and what could a reasonable way to proceed from his point of view.

As a side note, this is the tree of nested frames for the devtools toolbox:

• iframe (main toolbox frame) (this is where the type="content" attribute is being added from the attached patch)
  • iframe webconsole devtools panel
  • ... (other natively integrated devtools panels)
  • iframe "extension defined devtools panels" (this is where webext-panels.xul is being loaded)
    • browser remote="true" remoteType="extension" (this is the XUL browser element created from webext-panels.js, and where
      the extension page for the panel is being loaded, and it is meant to run in the extension child process)

(In reply to Luca Greco [:rpl] from comment #3)

...
With this test I noticed that the XUL browser custom element is raising a exception when webext-panels.js creates the remote XUL browser to run the "devtools panel extension page" in the extension process and adds it to the DOM tree, and at that point the XUL browser custom element throws a NS_ERROR_UNEXPECTED error while calling ssm.getLoadContextCodebasePrincipal(aboutBlank, this.loadContext); here:

• https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/toolkit/content/widgets/browser-custom-element.js#1054

I added some additional logging right above that line to compare what this.frameLoader is with and without the "frame.setAttribute("type", "content"); added to toolbox-hosts.js" and see why ssm.getLoadContextCodebasePrincipal may be throwing, and I noticed that the frameLoader.loadContext seems to be null with that change, and it is not null without it.

Here are some additional details about the differences that I notice on frameLoader value for the "devtools_panel extension page" XUL browser, with and without the change in toolbox-hosts.js:

• browser.frameLoader.browserContext:

  • with change in toolbox-hosts.js: null
  • without: CanonicalBrowserContext { ..., currentRemoteType: "extension", ...}

• browser.frameLoader.messageManager:

  • with change in toolbox-hosts.js: ChromeMessageSender { processMessageManager: null, remoteType: "" }
  • without: ChromeMessageSender { processMessageManager: ProcessMessageManager, remoteType: "extension" }

• browser.frameLoader.tabParent:

  • with change in toolbox-hosts.js: null
  • without: XPCWrappedNative_NoHelper { ... }

My guess is that the actual underlying issue is likely related to these differences in the XUL browser element frameLoader, and I think that it would be helpful to ask :bgrins opinion on these differences and what could a reasonable way to proceed from his point of view.

Thanks for the analysis Luca.

So as best as I can tell the frameLoader getter on a <browser> element isn't implemented in the Custom Element, but rather by the element itself (via XULFrameElement: https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/dom/chrome-webidl/XULFrameElement.webidl#19 and MozFrameLoaderOwner: https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/dom/webidl/MozFrameLoaderOwner.webidl#15-16).

So I think the places to look would be related to calls to nsFrameLoaderOwner::SetFrameLoader https://searchfox.org/mozilla-central/search?q=symbol:_ZN18nsFrameLoaderOwner14SetFrameLoaderEP13nsFrameLoader&redirect=false.

Nika, is there anything we should be watching out for when setting [type=content] that would be causing this type of problem (Comment 3 has a good breakdown of the symptoms but it seems like the root problem here is that browser.frameLoader is returning null on a bunch of getters with the [type=content] change with the patch in Bug 1539979).

So I spoke with Nika about this, and long story short we think we are hitting a special case here that's also being hit by about:addons (basically, a type="content" remote="true" frame inside of a type="content" frame). We may want to ultimately change the default behavior, but would like to wait until after some Nika's ongoing work finishes.

In the meantime, could you check around that function to see if the problem is fixed by adding another exemption to: https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/dom/base/nsFrameLoader.cpp#2515-2534 to also check for whichever URL we are loading webext-panels.js into? I assume this is toolbox.xul, though you might have to do some logging in that function to see which URL exactly we are using here.

I did some quick testing locally and wasn't seeing this path get hit, but I'm wondering if we also need "type=content" on the tool frames themselves? https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/devtools/client/framework/toolbox.js#1860.

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

So I spoke with Nika about this, and long story short we think we are hitting a special case here that's also being hit by about:addons (basically, a type="content" remote="true" frame inside of a type="content" frame). We may want to ultimately change the default behavior, but would like to wait until after some Nika's ongoing work finishes.

In the meantime, could you check around that function to see if the problem is fixed by adding another exemption to: https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/dom/base/nsFrameLoader.cpp#2515-2534 to also check for whichever URL we are loading webext-panels.js into? I assume this is toolbox.xul, though you might have to do some logging in that function to see which URL exactly we are using here.

I did some quick testing locally and wasn't seeing this path get hit, but I'm wondering if we also need "type=content" on the tool frames themselves? https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/devtools/client/framework/toolbox.js#1860.

Good call, adding an exception there seems to fix running DevTools webextensions running OOP. I simply duped the about:addons exception for the webext-panels.xul URL. Maybe we can do something cleaner with a chrome-only forceRemote parameter? But we can see that later.

DevTools webextensions are still crashing when running with --disable-e10s (again with browser.messageManager is null). We could probably live without it if needed. I suppose this is mostly relevant for Firefox for android, and DevTools webextensions cannot be used in this environment. I'll still try to investigate this use case a bit more.

Depends on D27421

Attached https://phabricator.services.mozilla.com/D27426 which seems to fix DevTools WebExtensions with disable-e10s. Part of it makes sense (extending an exception dedicated to about:addons in ExtensionPolicyService.cpp). I am not sure about the FrameLoader changes however.

If I don't force mIsTopLevelContent, messageManager is undefined on browser.
But if we go through:

  if (!isInWebExtensionsPanel && mIsTopLevelContent &&
      mOwnerContent->IsXULElement(nsGkAtoms::browser) &&
      !mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::disablehistory)) {
    // XXX(nika): Set this up more explicitly?
    nsresult rv = mDocShell->InitSessionHistory();
    NS_ENSURE_SUCCESS(rv, rv);
    mParentSHistory = new ParentSHistory(this);
  }

then this messageManager will be uninitialized.
I suppose something is off in this patch, maybe Nika knows how to properly handle this?

Yup, this is an unfortunate but sorta-known issue. I have a fix for the root issue in https://phabricator.services.mozilla.com/D27515, which should prevent the need for the webext panel special case. Could you try adding in those changes and seeing if stuff works out better?

Sorry for the delay, had some issues with my build environment.

I imported the complete queue linked to https://phabricator.services.mozilla.com/D27515 but it doesn't seem to help. I tried to remove my changes one by one, to see if any of them was now redundant. But my test always failed unless all the changes from the patches I attached here were included.

I can try again once the whole stack lands.

Depends on D33283

Depends on D33284

No longer necessary after Bug 1505909

Philipp: we will probably have to stop running DevTools webextensions in non-oop/non-e10s. Do you know if there are any DevTools webextensions used when debugging Thunderbird? I am not sure how the toolbox is connected to thunderbird actually, so maybe this is an irrelevant question (if it's a remote toolbox, devtools webextensions are not supported anyway)

https://hg.mozilla.org/mozilla-central/rev/754c35e948c3
https://hg.mozilla.org/mozilla-central/rev/74c4964d0842
https://hg.mozilla.org/mozilla-central/rev/0b30c9041216

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

Philipp: we will probably have to stop running DevTools webextensions in non-oop/non-e10s. Do you know if there are any DevTools webextensions used when debugging Thunderbird? I am not sure how the toolbox is connected to thunderbird actually, so maybe this is an irrelevant question (if it's a remote toolbox, devtools webextensions are not supported anyway)

I'm not aware of any devtools WebExtensions for Thunderbird. It is a remote toolbox at the moment like the browser toolbox, as we don't have e10s enabled and the toolbox in a tab hence does not work. Thunderbird will have to enable e10s at some point soon though since non-e10s support is going away in some areas. Thanks for keeping us in mind!