Closed Bug 1401538 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file)

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
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
Attachment #8916972 - Flags: review?(kechang)
Duplicate of this bug: 1406613
Comment on attachment 8916972 [details] [diff] [review]
v1

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

Looks good!
Attachment #8916972 - Flags: review?(kechang) → review+
Keywords: checkin-needed
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
Attachment #8916972 - Flags: approval-mozilla-beta?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/975e9765d6ed
Drop mTimerScheduledAt at RequestContext::CancelTailedRequest when tail queue is empty. r=kershaw
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/975e9765d6ed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8916972 [details] [diff] [review]
v1

Recent regression, Beta57+
Attachment #8916972 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1406091
Duplicate of this bug: 1405253
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...
Flags: needinfo?(honzab.moz)
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.
Flags: needinfo?(honzab.moz)
Duplicate of this bug: 1422275
You need to log in before you can comment on or make changes to this bug.