1640689 - Review usage of an html iframe to load arbitrary html content in netmonitor's preview sidebar

Status: RESOLVED FIXED

(DevTools :: Netmonitor, task, P2)

Description

[Alexandre Poirot [:ochameau]]

Bug 1637869 is highlighting that the Network Monitor is trying to load arbitrary HTML content via an HTML Iframe over here:
https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/devtools/client/netmonitor/src/components/previews/HtmlPreview.js#20-23

    iframe({
      sandbox: "",
      srcDoc: typeof htmlBody === "string" ? htmlBody : "",
    })

DevTools frontend is running in the parent process, with chrome privileges.
First, the "Toolbox", chrome://devtools/content/framework/toolbox.xhtml, is loaded in browser.xhtml via a XUL Iframe:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/devtools/client/framework/toolbox-hosts.js#412-413
It is using type="content", but as we are loading a chrome:// URL, we are still running with chrome privileges.
Then, the network monitor is loading chrome://devtools/content/netmonitor/index.html document in a another, nested, XUL Iframe:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/devtools/client/framework/toolbox.js#2482
And finally, we add these third iframe I mentioned in HtmlPreview component. This time an HTML one, which tries to use sandbox attribute.

Should we try to run this arbitrary code, i.e. content of HTML served by the webpage:

• in a content process?
• with content privileges? Is sandbox working as expected?

<xul:browser type="content" remote="true"> sounds like a better and safer option here, compared to <iframe sandbox>?
I quickly tried to use a <xul:browser> from this HtmlPreview component, but I'm not sure that the browser custom element is designed to work from such nested html document.
Would there be some attributes available on html Iframes equivalent to type=content and remote=true, at least for html iframes running in the parent process, from chrome documents??

Otherwise I have some doubt regarding this feature.
Any inner resource (JS, Image, Iframes) displayed in the previewed HTML document may be re-fetched from the network and be different from what has been recorded by the network monitor.
Should the preview prevent fetching any inner resource ? (I think there is a few handful attributes on DocShell that may be helpful)
Or should the preview ensure fetching the cached resource ? The one we recorded and are displaying in the netmonitor ? (Sounds more complex!)

Comment 1

[Alexandre Poirot [:ochameau]]

An additional note to followup on all what has been said on bug 1637869.

This crash/failure from bug 1637869 isn't related to DAMP helpers.
I confirm that it comes from the load of:
http://web.bild.de-talos/fis/tp5n/bild.de/web.bild.de/cont/schlaglichter/teaser.html@ifw=476&amp;ifh=47&amp;ifs=no&amp;ct=Bild-Channel+Home.html
But this isn't being loaded via addTab/addTrustedTab, nor any DAMP test script.
It is indirectly loaded from the test script, very precisely from here:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/testing/talos/talos/tests/devtools/addon/content/tests/netmonitor/netmonitor-helpers.js#137
This call will open the HTML Preview sidebar, and try to load the bild.de HTML content in the sidebar, via the <html:iframe sandbox>, and this HTML content includes a regular html <iframe> loading the http://web.bild.de-talos/fis... URL. This is where the test fails.

Comment 2

[Alexandre Poirot [:ochameau]]

Honza, it would be great to unblock jya. (This prevent landing bug 1637869) Do you have any suggestion/feedback about this?

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1640689 - [devtools] Use content and remote iframe for loading HTML preview in netmonitor.

But it looks like <xul:browser> doesn't like netmonitor setup, or creating it dynamically like this?

Comment 4

[Alexandre Poirot [:ochameau]]

@bomsy suggested me to try with <xul:iframe> as using <xul:browser> lead to such error:

JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 1212: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIScriptSecurityManager.getLoadContextContentPrincipal]

And the browser element doesn't display anything.
Unfortunately, xul:iframe doesn't throw any error, but still doesn't display anything.

The STR to trigger this HtmlPreview code is to open the netmonitor, load an html page, click on the request for the html page and select the Preview sidebar on the right.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1640689 - [devtools] Refactor HtmlPreview to create the iframe dynamically

This is just to allow creating XUL Iframe/Browser, otherwise we can't create element from another xmlns from React.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1640689 - Try xul:iframe

No exception, but still display nothing.

Comment 7

[Alexandre Poirot [:ochameau]]

Ok, so, the issue is about <browser type=content> being used for the Toolbox document.
It prevents inner documents from using nested content browser element.

Nika pointed out that WebExtension is using such nested content browser elements and this only works thanks to this whitelist:
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/dom/base/nsFrameLoader.cpp#2509-2535
If we want to use nested browser element, and I think we do, we have to add netmonitor.xhtml URL to this whitelist.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1640689 - Add netmonitor to the whitelist of nested content browsers.

Comment 9

[Alexandre Poirot [:ochameau]]

Adding netmonitor document to the whitelist works and allows using xul:iframe for loading the preview!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6adaf24aafe7d3ab8c6aa8d44a9761a7fdfa31

This fixes running the preview in a content process (instead of parent) and with type=content (I imagine it may be more restrictive than sandbox=""?

But this still keep open questions on which we should followup. Any inner resource (JS, CSS, Iframe, Images, ...) will be refetched from the network. This can be even more confusing with remote debugging as this will use a different user agent! See end of comment 0 for more info about that.

Comment 10

[:Harald Kirschner :digitarald]

Any inner resource (JS, CSS, Iframe, Images, ...) will be refetched from the network.

Agreed, no requests should be triggered by previewing HTML sources.

Comment 11

[Alexandre Poirot [:ochameau]]

These patches fails on DAMP:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303944523&repo=try&lineNumber=1017
I imagine it triggers similar issue as jya is having, where the load event on the preview's iframe doesn't fire.
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/netmonitor/netmonitor-helpers.js#140

Comment 12

[Alexandre Poirot [:ochameau]]

Oops, wait scratch all that from comment 11, I made a typo and was still using an HTML iframe in the changeset I pushed to try.

Comment 13

[Alexandre Poirot [:ochameau]]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ab6677315e2250a2d9cdbfc57ad4c8c697fc3c

Comment 14

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=A5njCyEmTz-Wz6-EcCS8Zw-0&revision=d0ab6677315e2250a2d9cdbfc57ad4c8c697fc3c
Still the same DAMP failure, but it looks like I broke the sidebar completely.
Could we possibly have no mochitest covering this feature? All mochitest are passing, that's concerning.
I'll do a fresh build overnight to investigate this further.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I just reported a follow up:
Bug 1644095 - No HTTP request should be triggered by previewing HTML response

Honza

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Alex, are you still planning to work on this one? There are already patches attached. Would review help at this point?

Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Not active atm, marking as P2

Comment 18

[Alexandre Poirot [:ochameau]]

Try run after rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e84b9680d97ccb903488b2f3893621f2c6046146

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1640689 - [devtools] Extend coverage of HTML previews in the netmonitor.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52bbd332bdd9
Add netmonitor to the whitelist of nested content browsers. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4e39348144b3
[devtools] Refactor HtmlPreview to create the iframe dynamically r=bomsy
https://hg.mozilla.org/integration/autoland/rev/419078dcf414
[devtools] Use content and remote iframe for loading HTML preview in netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/5fa87e754895
[devtools] Extend coverage of HTML previews in the netmonitor. r=bomsy

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/52bbd332bdd9
https://hg.mozilla.org/mozilla-central/rev/4e39348144b3
https://hg.mozilla.org/mozilla-central/rev/419078dcf414
https://hg.mozilla.org/mozilla-central/rev/5fa87e754895