Make view-source channel forward itself to redirect observers

ASSIGNED
Unassigned

Status

()

Core
ImageLib
ASSIGNED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86_64
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 682634 [details]
testcase

###!!! ASSERTION: Got a channel redirect for an unknown channel!: 'mChannel == oldChannel', file image/src/imgRequest.cpp, line 999
(Reporter)

Comment 1

5 years ago
Created attachment 682635 [details]
stack
Hmm, stack shows that imagelib is caching a link to the necko channel when it opens it, but the redirect call's "oldchannel" is different than that link.  

Do you know by any chance if this load hit a redirect chain (i.e more than one redirect)?
It might be possible that mChannel fails to get updated somehow during the 1st redirect, so when the 2nd happens 'oldchannel' is different than mChannel.  I don't see an obvious way that could happen from a glance at the code--it looks correct.  But this still seems more likely than the HTTP stack manufacturing new 'oldchannels' out of thin air...
Component: Networking → ImageLib
hmm, testcase uses a view-source:// URI.  Might be some interaction there, though I don't see any special handling of redirects in nsViewSourceChannel.cpp.  Don't have time to poke into this any more right now.
Created attachment 683367 [details] [diff] [review]
v1: remove incorrect assertion.

So viewsource channels have an internal pointer to the real HTTP channel.  Imagelib's mChannel points to the viewsource channel, while the redirect call's 'oldchannel' points to the real HTTP channel.

I thought of fixing the assert to first try QI-ing to nsIViewSourceChannel and skipping the test if it does, but I'm guess there are other cases (addons creating channel wrappers?) where this kind of nesting can happen, so just removing the assert seems best.
Assignee: nobody → jduell.mcbugs
Status: NEW → ASSIGNED
Attachment #683367 - Flags: review?(bzbarsky)
Someone familiar with imglib should also check that it's kosher for a redirect to change mChannel from the originating nsViewSourceChannel to the redirected-to nsHttpChannel.  That might be wrong.  The only question mark I see from looking at the code is that when we grab the URI of the redirected channel here

  http://mxr.mozilla.org/mozilla-central/source/image/src/imgRequest.cpp?rev=278bf5deb265#1052

it will have the view-source:// protocol stripped off.  But I don't think that causes the check for an invalid external app-opening protocols (mailto, etc) to fail--it actually might be needed to make it work?
Hmm, for some reason imgRequest also changes the callback object during OnStopRequest:

     http://mxr.mozilla.org/mozilla-central/source/image/src/imgRequest.cpp?rev=278bf5deb265#277

I'm unclear on what that accomplishes (we also do it if AsyncOpen fails--see line 275, but in that case it breaks a ref cycle--I wouldn't expect it would make any difference in OnStop since the HTTP channel will always break the cycle after calling OnStopRequest, via ReleaseListeners()).   But in the view-source redirect case the callback switch is going to happen on the new HTTP channel, so the view-source channel will keep pointing at the imgRequest (until it releases it in ReleaseListeners). Harmless?

If not, perhaps we shouldn't be updating mChannel during redirects if mChannel != oldchannel.
Comment on attachment 683367 [details] [diff] [review]
v1: remove incorrect assertion.

r=me I guess, but view-source is slightly cheating here.  Arguably it should be its underlying channel's redirect observer and forward itself over to redirect observers.  And in particular, there are other redirect observers that might assume the incoming channel is the thing they already know about.  :(
Attachment #683367 - Flags: review?(bzbarsky) → review+
> since the HTTP channel will always break the cycle after calling OnStopRequest

That wasn't always the case.... Worth checking the blame for why that code is there.
(Reporter)

Updated

5 years ago
Summary: "ASSERTION: Got a channel redirect for an unknown channel!" → "ASSERTION: Got a channel redirect for an unknown channel!" with view-source: redirect
Sounds like the right fix here is to implement comment 7 rather than ignore the assertion.

That said this isn't breaking anything known at the moment, and I don't know when I'll get to it.
Assignee: jduell.mcbugs → nobody
Summary: "ASSERTION: Got a channel redirect for an unknown channel!" with view-source: redirect → Make view-source channel forward itself to redirect observers
Duplicate of this bug: 813905
Attachment #683367 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.