Closed Bug 1385358 Opened 7 years ago Closed 7 years ago

Write Gmail site patch to disable http2

Categories

(Web Compatibility :: Interventions, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: miketaylr, Assigned: denschub)

References

()

Details

See Bug 1380896.

It sounds like we might be able to work around this issue by disabling http2 for gmail (and only gmail, ideally).
network.http.spdy.enabled.http2 is the pref.
Priority: -- → P1
This is actually a bit more complicated since we can't simply switch prefs for "one site". In theory, I could develop something hacky that turns the pref off when a request to gmail is started and flip it back on some time after that, but this does not seem to be very reliable. I wonder if there is a nicer way to do that.

Honza, since you seem to be working on that part of Firefox, I hope you can help me out. Initially, I had the idea of observing something like `http-on-modify-request` to get a reference to the http channel and toggle http2 there. If I understand the sources correctly, it should be possible to call HttpBaseChannel::SetAllowSpdy(false) or to set nsHttpHandler's mHttp2Enabled to false to have that particular request not using http2. However, I struggle to find a way to do that from inside an extension. I sure can listen to notifications like `http-on-modify-request`, but that only yields me a nsIHttpChannel instance, which isn't quite the right interface to disable http2 on. Also, this might actually be a bit late already.

Any hints?
Flags: needinfo?(honzab.moz)
Had a chat with Honza. Bug 1381016 might actually be the same issue, and they invented a solution for that an hour ago. I will build this patch locally to see if it fixes gmail. Also, Honza will provide updates and feedback tomorrow here in this bug, so keeping the ni? alive.
(In reply to Dennis Schubert [:denschub] from comment #2)
> This is actually a bit more complicated since we can't simply switch prefs
> for "one site". In theory, I could develop something hacky that turns the
> pref off when a request to gmail is started and flip it back on some time
> after that, but this does not seem to be very reliable. I wonder if there is
> a nicer way to do that.
> 
> Honza, since you seem to be working on that part of Firefox, I hope you can
> help me out. Initially, I had the idea of observing something like
> `http-on-modify-request` to get a reference to the http channel and toggle
> http2 there. If I understand the sources correctly, it should be possible to
> call HttpBaseChannel::SetAllowSpdy(false) or to set nsHttpHandler's
> mHttp2Enabled to false to have that particular request not using http2.
> However, I struggle to find a way to do that from inside an extension. I
> sure can listen to notifications like `http-on-modify-request`, but that
> only yields me a nsIHttpChannel instance, which isn't quite the right
> interface to disable http2 on. Also, this might actually be a bit late
> already.
> 
> Any hints?

Hmm.. better is to listen to on-opening-request and you can queryinterface the request (the channel) to nsIHttpChannelInternal where allowSpdy attribute is set.

However, I would rather wait since we may be on a way to have a patch in the bug you mention in comment 3.

Selectively disabling h2 might be a shortterm workaround, but performance will suffer because of that.
Flags: needinfo?(honzab.moz)
If bug 1381016 doesn't fix both itself and bug 1380896, we'll want to be able to disable H/2 on a per-site basis and land it in 56 if at all possible.

Talked to Dennis.  Plan is to have him write a JS-based fix that uses on-modify-request (or on-opening: shouldn't make any difference? I'd try on-modify first as it's more standard, then try on-opening-request if that doesn't work. But either should modify the channel before it hits the net).

If there's time we'll optimize the code to use C++ deeper in the necko stack.
Oh, also, per-site H/2 disabling will be a good tool to have around even if this particular incident resolves without needing it. It's quite possible major sites will roll out broken H/2 servers in the future.
So, RyanVM did some tests, and it seems like bug 1381016 fixes the GMail issue as well. I also tested the patch and I could not get GMail to hang with http2 enabled. That bug is tracking-firefox55+, so I am confident that this will land and be uplifted soon.

With the great hint by Honza to look for nsIHttpChannelInternal, I created a prototype that can disable http2 based on rules similar to the way we override user agents, see [0]. This lacks some tests, but it works.

(In reply to Jason Duell [:jduell] (needinfo me) from comment #5)
> I'd try on-modify first as it's more standard, then try on-opening-request if
> that doesn't work.

Actually, it doesn't work if I go for on-modify-request for any reason, which make me think I'm possibly doing something racy here. Need to put some time into investigating when the notifications are actually sent.

_However_, I wonder what's the right way to continue here. As far as I can tell, our immediate issue is resolved by the patch mentioned before, so we have nothing important to fix _right now_. While I agree that it might be good to have something ready if we run into similar cases again, I'd be much happier with spending more time on this code, to have a more polished version. Given that our current emergency is resolved and we don't have anything coming up in that regards, I'd rather focus on other tasks we planned on having done for 57.

Mike, what's your opinion here? Spending more time with getting the JS-side ready so that the Necko team can possibly apply some optimizations on a deeper level or let this rest for now, and add the http2 disable feature to our version 3 timeline. In case of an emergency, we'd still have the work I have prepared now.

[0]: https://github.com/mozilla/webcompat-addon/compare/master...denschub:bug1385358-http2-disabler(In reply to Jason Duell
Flags: needinfo?(miket)
(working link...)

https://github.com/mozilla/webcompat-addon/compare/master...denschub:bug1385358-http2-disabler

Yeah, I agree with you Dennis. Can you file a follow up bug to land a whitelist-based http2 disabler mechanism, in conjection with necko folks? Ideally we can test it at that point too. Thanks!
Flags: needinfo?(miket)
See Also: → 1386640
Filed bug 1386640 for landing the "clean" version. Since bug 1380896 should be fixed after landing 1381016, I'll go ahead and close this as a WONTFIX for now. I will monitor all bugs, and if it turns out we actually need to send out an emergency http2-disabler, I'll reopen and finish the work.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
No longer blocks: 1380896
You need to log in before you can comment on or make changes to this bug.