Closed Bug 579122 Opened 11 years ago Closed 11 years ago

imgRequestProxy race condition can fire 2 OnStartDecodes in a row


(Core :: ImageLib, defect)

Not set



Tracking Status
blocking2.0 --- beta5+


(Reporter: bholley, Assigned: bholley)




(2 files, 1 obsolete file)

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:

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:

We get a cache hit, and thus validate the entry:

We decide that, norly, we really want to validate, and call ValidateRequestWithNewChannel:

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.

We then proceed to instantiate an asynchronous imgCacheValidator, and return.

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()

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
Attached patch patch v1Splinter Review
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)
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+
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+

Resolving fixed.
Closed: 11 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.
Resolution: FIXED → ---
This seems to have stuck. Re-resolving fixed.
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.