77906 - OnStateChange doesn't report the images

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[ian holsman]

I've got the following code in the OnStateChange function in 
WebBrowserChrome.cpp.
and it fails (with nochannel) when a image request  comes. CSS/JS/HTML files 
work fine. 
This used to work fine about a month ago.
--


	nsCOMPtr<nsIChannel> channel;
	channel=do_QueryInterface(aRequest);
	if (!channel) {
	    	fprintf(stderr, "no channel\n");
	    	
		return NULL;
	}

	
	nsCOMPtr<nsIURI> uri;
	channel->GetURI( getter_AddRefs(uri));
	if (!uri) {
	    	fprintf(stderr, "Failed to get URI\n");
		return NULL;
	}

	nsXPIDLCString uriString;
	uri->GetSpec(getter_Copies(uriString));
	if (!uriString) {

	    	fprintf(stderr, "Failed to get spec\n");
		return NULL;
	}
	*aString = nsCRT::strdup((const char*)uriString);
	if ( *aString == NULL ) {
	    	fprintf(stderr, "Failed to strdup\n");
	}
	    	fprintf(stderr, "returned %s\n",*aString);
	return *aString;
}

Cheers
Ian

Comment 1

[Adam Lock]

Works for me pre and post 0.9 branches.

Have you set a breakpoint on the method to examine aRequest? There should be no 
reason for the QI to fail unless the aRequest pointer is nsnull, points to some 
other kind of object or perhaps the proxy is screwed up.

If it's any of these things, it may be instructive to walk up and down the stack 
a bit to find out why it is so.

Comment 2

[ian holsman]

I rebuilt it, and it definatly has a request field not null.
I am doing a distclean, and rebuiding everything from scratch, and seeing
if the problem still happens, then I'll start with the debugger

Comment 3

[ian holsman]

Ok...
the URL seems to get dropped in imgLoader::LoadImage when it creates the 
request.
(but I have no clue at this level, It all looks greek to me)

Comment 4

[Judson Valeski]

I'm seeing this too in mozilla. inline images are not getting causing 
OnStateChange() notifications to propegate. I suspect this is an artifact of 
images no longer using the uriLoader for loading. cc'ing pavlov.

Comment 5

[Judson Valeski]

rpotts and I were talking and are thinking that this is a result of one of the
following:

1. imagelib isn't adding it's requests to a loadgroup.
2. imagelib isn't adding it's requests to the correct loadgroup.
3. imagelib is adding it's requests to a loadgroup w/ the background flag set
(which would remove it from OnStateChange()) callbacks.

Comment 6

[Stuart Parmenter]

it is adding itself to all of those.  it isn't doing anything with
notificationcallbacks though.  is that the problem?

Comment 7

[Judson Valeski]

http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgRequestProxy.cpp#91
is where imagelib is adding it's requests to loadgroups. I don't know what this
proxy object is for, but I'll assume it corresponse to *every* image request. If
that's true, then imagelib is indeed adding all of it's requests to loadgroups.
The loadgroup comes in from the imglib caller via imgILoader::LoadImage*() calls
which users of imglib use to initiate imglib loads.

So, that either means, callers aren't handing imglib a good loadgroup, or that
the flag manipulation when creating a new channel is getting botched somehow
here http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgLoader.cpp#122 .

Comment 8

[Judson Valeski]

forgot to ans. your question. I don't think notificationcallbacks impact
OnStateChange() notifications. Rick?

Comment 9

[Stuart Parmenter]

a proxy represents each individual request.. those get added to the loadgroup of
the requestor.

the channel itself is not in any loadgroup.  perhaps this is the problem.  if
so, i don't know how to fix it.  the channel can't be added to a loadgroup,
since you could have multiple requests with different loadgroups... adding the
channel to the loadgroup would also mean that you could cancel the channel
directly, which would break different document, same image, different loadgroup
stuff.

Comment 10

[Stuart Parmenter]

oh.  well, my proxy objects do not impl nsIChannel, only nsIRequest.  If the
code in the initial description of this bug is what is happening, the QI will
always fail.  just another guess.. *shrug*

Comment 11

[rpotts (gone)]

The notification callbacks (nsIProgressEventSink in particular) are used to
generate the OnStateChange(TRANSFERRING) notification and the
nsIWebProgress::OnProgress(...) notifications.

It would sure be nice to get progress info for image downloads :-)

Comment 12

[Stuart Parmenter]

I actually disagree.  I think that sending progress notifications for images
will slow down performance due to far more progress notifications (its not as if
we don't already get WAY too many).

Comment 13

[Judson Valeski]

uh, I think image level progress is a pretty basic feature.

Comment 14

[Judson Valeski]

On thing that is not clear to me w/ this, is although we're not producing image
level onStateChange notifications, the mozilla browser UI seems to produce a
progress meter that includes the images (ie. it doesn't complete until all the
images have come in).

the docloader produces nsIWebProgressListener callbacks based on
nsIProgressEventSink, therefore I'm starting to wonder if this is fallout from
the fact that imagelib no longer creates channels, rather than it not providing
notification callbacks. I wonder if we're failing in the docloader somewhere
trying to get a channel for an image, and subsequently we're not producing an
OnStateChange() for it.

Comment 15

[rpotts (gone)]

get a grip pav...  it's called "user experience".  rendering the image slows 
down the download too :-)

Sending progress notifications require *one* method call!  Since images are the 
bulk of HTTP transactions, not sending progress info makes the whole progress 
notification mechinism useless!

Comment 16

[Judson Valeski]

cc'ing darin (explicit question for you below)

I was right in that we are indeed getting the OnStateChange() notification for
image requests. The opperative word there is _requests_. imagelib doesn't impl
channels which is what most OnStateChange() consumers are trying to use to get
some user presentable string looking like a URI out of the callback.

So, the semantic for nsIWebProgressListener implementors who want to get a uri
string out of the requests that come in through the callbacks is that they need
to call the nsIRequest.idl "name" attribute (->GetName(PRUnichar **)) on the
request. That reflects some meaningful string for presentation. 

This semantic has the following affect on callers... where they were once using
"char*" w/ nsIURI->GetSpec(), they now need to use "PRUnichar*" w/
nsIRequest->GetName(). Darin, we were talking about the problem of exposing URI
specs as char* or PRUnichar* a week or two ago. Enforcing this new model would
obviously force users into the "uri's are PRUnichar*'s at the UI level" world.
Thoughts?

This model also requires mozilla to provide "names" that people want. Namely
URI's when possible. I'm attatching a patch to scrape some imagelib specific
strings out of the name attribute so we return a raw URI instead. 

Comment 17

[Judson Valeski]

Attachment: patch to make image request names more general

Comment 18

[Stuart Parmenter]

uh, sure.  r=pavlov

Comment 19

[rpotts (gone)]

sr=rpotts

Comment 20

[Judson Valeski]

patch has been checked in.

Comment 21

[Michael Dunn]

Clean up verification of dated code change bus