Closed Bug 1401538 Opened 2 years ago Closed 2 years ago
Timer Scheduled At at Request Context::Cancel Tailed Request when tail queue is empty
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  we must change the code to: // Must drop to allow re-engage of the timer mTimerScheduledAt = TimeStamp();  https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/netwerk/base/RequestContextService.cpp#397
Comment on attachment 8916972 [details] [diff] [review] v1 Review of attachment 8916972 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8916972 - Flags: review?(kechang) → review+
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/975e9765d6ed Drop mTimerScheduledAt at RequestContext::CancelTailedRequest when tail queue is empty. r=kershaw
Comment on attachment 8916972 [details] [diff] [review] v1 Recent regression, Beta57+
Attachment #8916972 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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...
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.
You need to log in before you can comment on or make changes to this bug.