If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

imgRequestProxy race condition can fire 2 OnStartDecodes in a row

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

There is a subtle race condition with the notification code when an image is first loaded that can lead to OnStartDecode being called twice in a row without getting OnStop* in between. This is causing an abort in the onload blocking code in nsImageLoadingContent.cpp:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp#197

This doesn't normally show up, but we do enough decode requests in bug 512260 that it starts to become in issue there. The fix for this bug depends on joe's work in bug 572520, so we need to wait for that.

Here's what happens:

A consumer wants an image. To this end, it calls LoadImage:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1491

We get a cache hit, and thus validate the entry:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1555

We decide that, norly, we really want to validate, and call ValidateRequestWithNewChannel:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1356

Next, we create a proxy, which (ahah!) adds the proxy to the imgRequest without sending any notifications, since we send those later during cache entry validation.
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1200
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgRequestProxy.cpp#131

We then proceed to instantiate an asynchronous imgCacheValidator, and return.
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1213

Next, before the cache validator gets itself going, we have some activity on the underlying image (say, someone calls RequestDecode() in bug 512260), which causes it to start decoding and send OnStartDecode(), but nothing farther than that.

The imgCacheValidator gets itself going, and calls NotifyProxyListener()
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#2021

BAM! 2 OnStartDecodes in a row!

Joe thinks that the solution here is to use his "defer" machinery in bug 572520, but that's not ready yet. So we wait.
This blocks bug 512260, which blocks a blocker (bug 563088). Nominating.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Who owns this?  Need an owner ASAP.
That's me! Forgot to take the bug - sorry about that.
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
Created attachment 462155 [details] [diff] [review]
patch v1

Added a patch to call defer/undefer in the right places. I spoke with Joe a bit about this on gchat, and he was ok with it. Flagging for review.
Attachment #462155 - Flags: review?(joe)
Created attachment 462196 [details] [diff] [review]
Initial stab at fixing the race condition
Comment on attachment 462196 [details] [diff] [review]
Initial stab at fixing the race condition

Oh hey, that actually worked! Sorry for the noise.

(In case anyone's interested, I just figured out how to push from git to bugzilla).
Attachment #462196 - Attachment is obsolete: true
pushed to try as 0b9b622cde81
Comment on attachment 462155 [details] [diff] [review]
patch v1

I'd prefer the comments say something along the lines of "We will send notifications from imgRequestValidator::OnStartRequest; in the mean time, we must defer notifications because we are added to the imgRequest's proxy list, and we can get extra notifications resulting from methods such as RequestDecode()." Just a little bit more explicit.

Also, is it possible for you to write a test along the lines of the ones I did for bug 572520, to make sure that we don't get two OnStartRequests in a row? I presume we can't just add NS_ABORT_IF_FALSEs to imgStatusTracker because of other latent issues along this line.
Attachment #462155 - Flags: review?(joe) → review+
Created attachment 462554 [details] [diff] [review]
race condition fix

Updated patch, addressing joe's comment concerns. I _think_ this looks good on try, but it's hard to tell because everything's so orange. I'll push it when things clear up.
This blocks beta 5, the feature freeze.
blocking2.0: betaN+ → beta5+
pushed:
http://hg.mozilla.org/mozilla-central/rev/58b623ff605c

Resolving fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
There's some tree weirdness going on right now (tinderbox is being crashy). I'm going to leave this open to make sure we verify that it stuck.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This seems to have stuck. Re-resolving fixed.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.