imgRequestProxy::CancelAndForgetObserver doesn't remove from imgCacheValidator proxy list

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

When I log into gmail with a debug build, I have been tripping an assert at:

https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/image/imgLoader.cpp#2954

Which happens after imgRequestProxy::CancelAndForgetObserver has been called on a proxy. It looks like we never remove cancelled proxies from the imgCacheValidator::mProxies array. Additionally we may clear imgRequestProxy::mDeferNotifications as part of imgRequest::RemoveProxy / ProgressTracker::RemoveObserver. This particular sequence of events should be harmless on release builds.
Assignee: nobody → aosmond
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Status: NEW → ASSIGNED
Duplicate of this bug: 1159285
Attachment #8927837 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e63329ef608
Ensure that imgRequestProxy::CancelAndForgetObserver removes itself from the cache validator. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/7e63329ef608
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this.
Flags: needinfo?(aosmond)
Comment on attachment 8927837 [details] [diff] [review]
0001-Bug-1416774-Ensure-that-imgRequestProxy-CancelAndFor.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 579122 introduced the assert we are tripping.
[User impact if declined]: None, because on release builds this should have no impact beyond some minor accounting.  This does fix intermittent test failures on debug however.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Small, well contained and well understood solution. Since this is on the teardown of a request, even if we missed some notifications as a result of this, they will have no impact, since the request was destroyed anyways.
[String changes made/needed]: None.
Flags: needinfo?(aosmond)
Attachment #8927837 - Flags: approval-mozilla-beta?
Comment on attachment 8927837 [details] [diff] [review]
0001-Bug-1416774-Ensure-that-imgRequestProxy-CancelAndFor.patch

Fix an assert issue. Beta58+.
Attachment #8927837 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.