1401538 - Drop mTimerScheduledAt at RequestContext::CancelTailedRequest when tail queue is empty

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

Thanks for taking care of bug 1397880!

However, there is one thing we need to fix.  You've added a change that drops the mUntailAt timestamp (wrong) but didn't add mTimerScheduledAt drop (correct).

mUntailAt is driven by the number of un-tailed requests which has not changed in RequestContext::CancelTailedRequest (we remove a tailed request).

mTimerScheduledAt MUST reflect the time the timer is expected to fire.  And it now has been unscheduled.  There is a 1-1 relation of the untail timer being and the mTimerScheduledAt: they are either both null or both set up, there is no other option.

hence, at [1] we must change the code to:

  // Must drop to allow re-engage of the timer
  mTimerScheduledAt = TimeStamp();


[1] https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/netwerk/base/RequestContextService.cpp#397

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8916972 [details] [diff] [review]
v1

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

Comment 4

[Kershaw Chang [:kershaw]]

Comment on attachment 8916972 [details] [diff] [review]
v1

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

Looks good!

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8916972 [details] [diff] [review]
v1

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1397880 (fixing another regressions in bug 1358060)

[User impact if declined]:
pages not finishing load

[Is this code covered by automated tests?]:
no

[Has the fix been verified in Nightly?]:
yes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1406613#c12

[Needs manual test from QE? If yes, steps to reproduce]: 
this is very hard to repro reliably, so no.

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
it's a well defined and understood change isolated to the request tailing specific code (doesn't have a broad impact)

[String changes made/needed]:
none

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/975e9765d6ed
Drop mTimerScheduledAt at RequestContext::CancelTailedRequest when tail queue is empty. r=kershaw

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/975e9765d6ed

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916972 [details] [diff] [review]
v1

Recent regression, Beta57+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6fcfa157e5b8

Comment 12

[:Gijs (he/him)]

Can we add tests for this? Given all the favicon issues in bug 1406091 and dupes, and how hard it was to track those down, it would be really nice to ensure tests would catch issues like this...

Comment 13

[Honza Bambas (:mayhemer)]

This was about a very tight race.  To write a test that would be reliable (always fail when tailing is SOMEHOW broken) is extremely hard.  Hence, no, we could not add a test.