Closed Bug 1387090 Opened 2 years ago Closed 2 years ago

Introduce DontThrottle class of service flag properly

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

+++ 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).
Attached patch v1 (obsolete) — Splinter Review
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).
Attachment #8893456 - Flags: review?(mcmanus)
Blocks: 1386323
Attached patch v2Splinter Review
OK, this was not building actually.  The RemoveActiveTransaction method needs the argument for the UpdateActiveTransaction case, using Maybe<bool> to add an override.
Attachment #8893456 - Attachment is obsolete: true
Attachment #8893456 - Flags: review?(mcmanus)
Attachment #8893492 - Flags: review?(mcmanus)
No longer blocks: 1377206
Attachment #8893492 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c037eccf31b4
Properly handle DontThrottle flag in all places in nsHttpTransaction. r=mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c037eccf31b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
No longer blocks: 1377205
Depends on: 1377205
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
Attachment #8893492 - Flags: approval-mozilla-beta?
(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 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?
Flags: needinfo?(honzab.moz)
Attachment #8893492 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(honzab.moz) → needinfo?(lhenry)
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!
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.