1473519 - Switch off TRR while the devtool toolbox is up

Status: RESOLVED INVALID

(DevTools :: Framework, enhancement, P1)

Description

[--]

TRR is the DNS-over-HTTPS subsystem in Firefox, and because it changes how name resolves are done (to which server it sends its name lookup requests to) it will make web developers able to better work with /etc/hosts changes etc if TRR is automatically disabled when the devtool's toolbox is up.

Comment 1

[--]

Attachment: bug 1473519 - disable TRR while devtools' toolbox is active

MozReview-Commit-ID: 33mK3PHU8ss

Comment 2

[Patrick McManus [:mcmanus]]

I thought this was landed. I'd really like this on 63

Comment 3

[--]

Attachment: 0001-bug-1473519-disable-TRR-while-devtools-toolbox-is-ac.patch

Hi Alexandre,

I'm now back with a fresh take on this disabling of TRRR when the devtools are in use. I could use some advice on this take, as it seems this doesn't seem to actually work.

News since my previous attempt is that we can now disable TRR with nsIRequest loadflags, which I though was how the regular cache was already enabled/disabled in the devtools? 

Any hint on what I do wrong here?

Comment 4

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Comment on attachment 9003043 [details] [diff] [review]
0001-bug-1473519-disable-TRR-while-devtools-toolbox-is-ac.patch

Review of attachment 9003043 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/targets/browsing-context.js
@@ +1056,1 @@
>      }

I have no idea how to test that. So it is hard for me to see if/why it works/doesn't work.

But overall the patch looks much better on the devtools side of things. I'm quite hesitant to disable it, like this, completely hardcoded, without any way to prevent this auto-disabling.
Shouldn't we let a change to have the devtools with regular TRR behavior?

Otherwise, codewise, it may be easier to do only that, rather than modifying setCacheDisabled:
I have no idea how to test that. So it is hard for me to see if/why it works/doesn't work.

But overall the patch looks much better on the devtools side of things.
```
this.docShell.defaultLoadFlags |= Ci.nsIRequest.LOAD_DISABLE_TRR;
```

Finally, about why it could possibly not work. This code is called on toolbox opening, so the page already loaded won't have been loaded with this flag. Only the next reload/pages will. Now it may rather be an issue with defaultLoadFlags not working as expected with LOAD_DISABLE_TRR, that I don't know...

Comment 5

[--]

> Shouldn't we let a change to have the devtools with regular TRR behavior?

I wouldn't mind having that ability, probably with the default being to disable-TRR, sure. That would be neat.

> I have no idea how to test that. So it is hard for me to see if/why it works/doesn't work.

Sorry, I've added some logging now and I believe it *does* the right thing and the problem is rather in my end. I'll investigate and get back.

BTW, to test:

 1. set the prefs network.trr.mode to "2" and network.trr.uri to "https://mozilla.cloudflare-dns.com/dns-query"
 2. pop up about:networking in a tab, click the DNS option. This shows the DNS cache and there's a TRR column that should start to say 'true' for names when TRR is used (if you click autorefresh it'll keep updating)
 3. when using the devtools, the names resolved in that tab should end up with TRR false in the about:networking tab

Comment 6

[--]

Okay, here's why it doesn't work (disclaimer: I know very little of how the devtools actually works and gets its magic done):

When the devtools sets LOAD_DISABLE_TRR in its default loadflags, that bit is certainly passed in on the request issued by devtools.

However: Immediately before that devtools request comes the real(?) request which uses TRR (if I have that preffed on), which kicks off the name resolve so when the devtools request arrives just afterward (with TRR-disabled) there's already a name resolve in progress and the second one just "piles on" the previous one to get the result when that is completed. But that name resolve is then using TRR...

Ideally we want the initial request to get done without TRR but I'm not sure how we can do that without changing the pref, which is global and thus would affect all tabs and not only the devtools-using tab! :-/

Comment 7

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Looking at your journey, it looks more like an issue in defaultLoadFlags + Ci.nsIRequest.LOAD_DISABLE_TRR rather than anything DevTools-related. I don't think there is anything DevTools can do to mitigate that.
DevTools code can toggle the preference and have an application-wide effect, but that would be unfortunate to end up having to do that.

Just a random thought: can we "purge" the resolutions somehow, so that the second, devtools request has to re-resolve?

Comment 8

[Alexandre Poirot [:ochameau] PTO, back May 4th]

(In reply to Daniel Stenberg [:bagder] from comment #5)
> > Shouldn't we let a change to have the devtools with regular TRR behavior?
> 
> I wouldn't mind having that ability, probably with the default being to
> disable-TRR, sure. That would be neat.
> 
> > I have no idea how to test that. So it is hard for me to see if/why it works/doesn't work.

Otherwise, about that you may take the cache disabling as an example.

We add a checkbox in DevTools settings panel like this:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.xhtml#164-169
We watch the pref changes to update this UI like this:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-options.js#101,119,133-137

We call reconfigure whenever this pref change from the toolbox here:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1329-1341
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#508
And also watch for this pref changes from here:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#479

Comment 9

[--]

> Just a random thought: can we "purge" the resolutions somehow, so that the second, devtools request has to re-resolve?

That would be one way, but we don't currently have a way to nuke an ongoing resolve so that would require some fiddling.

I'm testing out a different method now: make the resolver consider TRR and non-TRR results to be different when looking up the entry in the cache.

Comment 10

[--]

Patrick,

Right now (with this patch), when running devtools when TRR-enabled, TRR will be used by necko itself when resolving host names but the devtools will then redo the requests with TRR-disabled immediately afterward.

I would've wanted a concept where the entire sequence would've been done TRR-disabled for this scenario, but I can't seem to come up with a working method that isn't simply "disable the TRR pref".

Do you have any thoughts or added considerations?

Comment 11

[--]

Attachment: v2-0001-bug-1473519-disable-TRR-while-devtools-toolbox-is.patch

I simplified the patch somewhat...

Comment 12

[Patrick McManus [:mcmanus]]

I don't really understand what comment 10 means.

when connecting to www.example.com in dev tools, does the http response shown come from the host provided by TRR or by the OS. 

It needs to be the latter.

Comment 13

[--]

Alexandre,

Can you just clarify how this works in the devtools? There are now two requests for the URL I give in the address bar when I have devtools open, the first uses TRR (since I have it pref'ed on) and the second one (from devtools) has TRR disabled with my patch.

Assuming these two operations would return different data, which of them is actually used/shown in the tab that I run devtools on?

Comment 14

[Alexandre Poirot [:ochameau] PTO, back May 4th]

With STR from comment 5 and the latest patch, I always get TRR=true in about:networking?

Is there any other way to debug that, for example stdout logging?

The first request is most likely the Tab request. But with your patch, the tab's docshell should have LOAD_DISABLE_TRR set and disable the TRR...
Any subsequent requests may be done by the tools to fetch resources. These requests are supposed to come from the cache in order to ensure retrieving the same data than the tab uses. All these requests should be done through this "fetch" helper method:
https://searchfox.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#535-538
You may want to tweak the loadFlags here to see if that fixes things for you.
It is far from being clear what happens with TRR in this helper method. I would expect to only fetch data from cache and not redo any actual request, but may be the loadFlags make it so that we redo one?

Comment 15

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Also, it would be easier if we could display information about TRR for each request in the network monitor.
Do you think it is possible? The network monitor has access to the nsIHttpChannel object for each request, could we fetch TRR status from it?

Comment 16

[--]

> I always get TRR=true in about:networking?

I get it true *most* of the time but not always, and the problem seems to be that my loadflags only have the DISABLE_TRR bit intermittently when my DNS code is called. I need to figure out why this is so. For a second I suspect the devtools code since in my simplified approach, the setCacheDisabled() function overwrites the default flags and unconditionally clears the DISABLE_TRR bit - but I've played with a version of the patch that doesn't and it seems to not help anyway! 

Most annoying!

> Is there any other way to debug that, for example stdout logging?

"MOZ_LOG=nsHostResolver:5 ./mach run"

But it should be noted that the logging is mostly to help us/me debug TRR so it'll spew quite a lot of TRR details.

I've also used this local patch to help me notice better when the DISABLE_TRR bit is passed in:

diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp
index e7e76bec7f08..586630ecdab9 100644
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -804,13 +804,14 @@ nsHostResolver::ResolveHost(const nsACString &aHost,
                             nsResolveHostCallback  *aCallback)
 {
     nsAutoCString host(aHost);
     NS_ENSURE_TRUE(!host.IsEmpty(), NS_ERROR_UNEXPECTED);
 
-    LOG(("Resolving host [%s]%s%s.\n", host.get(),
-         flags & RES_BYPASS_CACHE ? " - bypassing cache" : "",
-         flags & RES_REFRESH_CACHE ? " - refresh cache" : ""));
+    LOG(("Resolving host [%s]%s%s%s\n", host.get(),
+         flags & RES_BYPASS_CACHE ? " BYPASS_CACHE" : "",
+         flags & RES_REFRESH_CACHE ? " REFRESH_CACHE" : "",
+         flags & RES_DISABLE_TRR ? " DISABLE_TRR" : ""));
 
     // ensure that we are working with a valid hostname before proceeding.  see
     // bug 304904 for details.
     if (!net_IsValidHostName(host))
         return NS_ERROR_UNKNOWN_HOST;

> The network monitor has access to the nsIHttpChannel object for each request, could we fetch TRR status from it?

It's curently only present in the nsIHttpChannelInternal, but I suppose that's not good enough...

Comment 17

[Alexandre Poirot [:ochameau] PTO, back May 4th]

(In reply to Daniel Stenberg [:bagder] from comment #16)
> > The network monitor has access to the nsIHttpChannel object for each request, could we fetch TRR status from it?
> 
> It's curently only present in the nsIHttpChannelInternal, but I suppose
> that's not good enough...

We already query this interface from the netmonitor code:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/network-monitor/network-observer.js#305
So that sounds good. The main issue is that this attribute is flagged "noscript", so Javascript can't access this property...

(it is quite easy to add a new column in the netmonitor, Honza recently added a new one in bug 1333994)

Comment 18

[--]

> The main issue is that this attribute is flagged "noscript", so Javascript can't access this property...

All right, but then it struck me that this member actually means that the "channel is used by the internal trusted recursive resolver" ie the actual request that is sent to the DoH server. It does not mean that TRR was used to resolve the name when this HTTP transfer was setup. We need to add such a one, and I propose we do that in a separate bug.

Comment 19

[--]

I pushed a minor update of this patch to Phabricator now, so that it sets the bits better for the cache disable/enable and sets the default flags to DISABLE_TRR.

This approach works for me now since bug 1487432 landed.

Comment 20

[--]

Hi Alexandre,

Any suggestion on what to do next to drive this forward? Would you prefer an on/off option?

Comment 21

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Sorry for the delay, I was out on PTO for 2weeks.

> Any suggestion on what to do next to drive this forward? Would you prefer an
> on/off option?

TBH, I don't quite follow TRR development to know how bad it is to not being atble to have DevTools *and* TRR turned on.
I would say it is up to you/your team to make the call here.
But I would be happy to review a patch with an option like the ones for cache and service workers.

If you want to proceed with phabricator's patch, note that you should restore defaultLoadFlags on devtools shutdown.
I think you did it in a previous version of your patch.
See code in restoreDocumentSettings for examples:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#1079

Also, a test would be helpful to ensure we do not regress your feature but testing this via mochitests might be hard...

Comment 22

[Jason Duell]

After consulting with devtools folks we decided the best course of action here is for devtools to simply use the same setting (DNS-over-HTTPS is either on/off) as the browser.  No patch or additional UX needed.