473161 - Bug 373701 regressed on trunk and 1.9.1

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The patch that ended up landing for bug 89419 is wrong.  In particular, this part of the patch:

    1.60 -  if (mRequest && mLoading)
    1.61 -    mRequest->Cancel(aStatus);
    1.62 +  if (mChannel && mLoading)
    1.63 +    mChannel->Cancel(aStatus);

is wrong, because it cancels the wrong thing (the part channel, not the underlying multipart channel).

I really should have pushed harder to clean up the mRequest/mChannel thing so that confusion like this wouldn't arise.  :(

This needs to block 1.9.1, in my opinion.  And we desperately need a regression test for this if someone can figure out how to write one.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Cancel the request in Cancel(), and the channel in CancelAndAbort().

It's still necessary to cancel mChannel in CancelAndAbort() for the same reasons that came up in bug 89419, but we should cancel mRequest in Cancel() because, if we're calling Cancel(), we've initialized properly. Just cancelling mRequest will work because, on redirect, the new channel will be added to the old channel's load group.

It's not currently possible to write a test for webcams, but I've filed bug 475108 on it anyways.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm a little confused.  Why do we need to cancel the channel in this case?  If the channel is not in the loadgroup, that means its AsyncOpen failed, right?  In that case there's nothing to cancel...  Why is that call needed at all?

Comment 3

[Joe Drew (not getting mail)]

Attachment: Change from cancelling the channel to cancelling the request

That is a good question, and it turns out that there is no reason to cancel the channel instead of the request. I was misled down that path when debugging the leaks in bug 89419, and I made incremental changes until I fixed the leaks, never realizing that the Cancel() call didn't matter. So this simply reverts to the correct Cancel().

Comment 4

[Joe Drew (not getting mail)]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/7d9bea3fa51a

Comment 5

[Joe Drew (not getting mail)]

Backed out because of leaks.

Comment 6

[Joe Drew (not getting mail)]

Repushed, and seemed to stick: http://hg.mozilla.org/mozilla-central/rev/bf337d3b5347

Comment 7

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8e8806783ea3

Comment 8

[Daniel Veditz [:dveditz]]

I assume we don't need this fixed on 1.9.0 because Joe already caught the problem with bug 89419 in bug 355126 comment 65? (nominating wanted1.9.0.x as a reminder that I need to check back for an answer)

Comment 9

[Boris Zbarsky [:bzbarsky]]

We don't need this because the code that caused this bug didn't land on 1.9.0 at all.  We took a much simpler and safer fix for bug 89419 there instead.  At least that's how things work as I recall.  Joe, can you confirm?

Comment 10

[Joe Drew (not getting mail)]

Right. There was drift both in the underlying code from 1.9.0->1.9.1 and in the patch I eventually checked in. Therefore, this bug doesn't apply to the 1.9.0 branch.