Closed
Bug 1387090
Opened 7 years ago
Closed 7 years ago
Introduce DontThrottle class of service flag properly
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 1 obsolete file)
14.54 KB,
patch
|
mcmanus
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ 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).
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4031471f84965274750b839759b39da488092aed
Updated•7 years ago
|
Attachment #8893492 -
Flags: review?(mcmanus) → review+
Assignee | ||
Updated•7 years ago
|
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
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c037eccf31b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 6•7 years ago
|
||
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?
Assignee | ||
Comment 7•7 years ago
|
||
(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•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
(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)
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/37098c6bb11c
status-firefox56:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•