1387090 - Introduce DontThrottle class of service flag properly

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Honza Bambas (:mayhemer)]

+++ This bug was initially created as a clone of Bug #1377205 +++

There are two more places I missed to update (damn you search in visual studio).

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

I missed two places where only the Throttleable class of service flags check was passed as an argument (at the Add and Remove palces).  The new EligibleForThrottling() method that encapsulates the check has to be used instead.  To prevent such a mistake in future, I rather let the callee methods grab its value internally from aTrans rather than passing it from above (also may save some CPU).

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v2

OK, this was not building actually.  The RemoveActiveTransaction method needs the argument for the UpdateActiveTransaction case, using Maybe<bool> to add an override.

Comment 3

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4031471f84965274750b839759b39da488092aed

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c037eccf31b4
Properly handle DontThrottle flag in all places in nsHttpTransaction. r=mcmanus

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c037eccf31b4

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8893492 [details] [diff] [review]
v2

This is a regression fix for bug 1383234 and also a prerequisite for regression bug 1386323

Approval Request Comment
[Feature/Bug causing the regression]: bug 1383234
[User impact if declined]: potentially breaks the HTTP response intelligent throttling feature, but it's hard to assess how badly and what the effect can be
[Is this code covered by automated tests?]: not this feature in particular, but the code paths are well exercised by all our tests doing networking
[Has the fix been verified in Nightly?]: there is not much to verify here since the regression effect of bug 1383234 is hard to assess
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: bug 1377205 to land cleanly
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is mostly a refactoring and revert of the HTTP throttling feature to a state as before bug 1383234
[String changes made/needed]: none

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> [User impact if declined]: potentially breaks the HTTP response intelligent
> throttling feature, but it's hard to assess how badly and what the effect
> can be

This could potentially be memory leaks and/or a major slowdown of networking in an edge case.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8893492 [details] [diff] [review]
v2

This looks like a rewrite of work that already landed in 56 in bug 1383234.  The patch in bug 1377205 should land first. 

If these don't work out we may just need to back them out and back out bug 1383234 completely and aim the changes at 57 rather than 56.  Honza, is that ok with you?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> Comment on attachment 8893492 [details] [diff] [review]
> v2
> 
> This looks like a rewrite of work that already landed in 56 in bug 1383234. 

It's an update to it and somewhat a cleanup (not sure "rewrite" is the correct word here).  It looks big, but it's actually quite a simple and small change.  I could also create a minimized beta patch, but I don't think it's worth the time.

> The patch in bug 1377205 should land first. 

Yes, as mentioned in the a? comment and as you noted in that bug, right.

> 
> If these don't work out we may just need to back them out and back out bug
> 1383234 completely and aim the changes at 57 rather than 56.  Honza, is that
> ok with you?

What exactly means "don't work out" ?  

It's 50/50.  I'd prefer to land all the patches I asked for approval to have coverage, but if you find it to be a problem, back bug 1383234 out from beta.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Honza. I was just trying to say if there are any major regressions, we may want to back this out. It seems well worth trying on beta!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37098c6bb11c