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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1416774-Ensure-that-imgRequestProxy-CancelAndFor.patch

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e63329ef608
Ensure that imgRequestProxy::CancelAndForgetObserver removes itself from the cache validator. r=tnikkel

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7e63329ef608

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 7

[Gerry Chang [:gchang]]

Comment on attachment 8927837 [details] [diff] [review]
0001-Bug-1416774-Ensure-that-imgRequestProxy-CancelAndFor.patch

Fix an assert issue. Beta58+.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/af82d0c33427