86521 - Request passed to OnLocationChange() is always null

Status: RESOLVED WONTFIX

(Core Graveyard :: Embedding: APIs, defect)

Description

[David Epstein]

Tried this in mfcEmbed and TestEmbed, using 6/14 embed build.
 do steps 1-3 in source code:
1. Add a listener for web progress listening. AddWebBrowserListener() using IID
for nsIWebProgressListener.
2. Make sure that you have web prog lstnr methods implemented. 
3. In OnLocationChange(), add code for request handling. Example (this is what I
added. Note that aRequest is the (nsIRequest *) parameter input for
OnLocationChange()):

nsXPIDLString theReqName;
nsresult rv;

rv = aRequest->GetName(getter_Copies(theReqName));
if(NS_SUCCEEDED(rv))
{
   stringMsg.AssignWithConversion(theReqName);
   OutputFile("The request name = ", stringMsg.get(), 1);  // local method
}
else
   OutputFile("We didn't get the request name.");

4. launch embedding app.
5. If web prog listener (AddWebBrowserListener()) isn't called in your browser
init method, invoke method to call it now.
6. Enter a url.

Result: It tracks states in OnStateChange(), but when it goes into
OnLocationChange(), it crashes. We get an unhandled exception on this line:

rv = request->GetName(getter_Copies(theReqName));

Comment 1

[rpotts (gone)]

The reason this is crashing is because the request passed out through
OnLocationChanged(...) is *always* NULL.

Comment 2

[David Epstein]

changed qa contact to depstein.

Comment 3

[Judson Valeski]

is the request always being null a bug, or should we remove the arg altogether
from OnLocationChange()?

Comment 4

[rpotts (gone)]

At this point, i'm tempted to just remove the request argument from
OnLocationChange().

It looks like there will be situations where we do not have an associated
request (in the about:blank case in particular).  Also, the request is not that
'useful' in this notification...

I think that we should either inforce that the 'aRequest' argument ALWAYS refers
to the document channel (or null if the document was not created via a channel),
or just remove it all together.

If we remove the argument, we'll need to do it by mozilla 1.0.

-- rick

Comment 5

[David Epstein]

nominating topembed. Just comment out the RequestName line in TestEmbed
(http://lxr.mozilla.org/seamonkey/source/embedding/qa/testembed/BrowserImplWebPrgrsLstnr.cpp#265)
and recompile. Enter a url that redirects like cisco.com. Here's the crash log:


RequestName(nsIRequest * 0x00000000, nsCString & {...}, int 1) line 221 + 7 bytes
CBrowserImpl::OnLocationChange(CBrowserImpl * const 0x00eb2300, nsIWebProgress *
0x00e96b04, nsIRequest * 0x00000000, nsIURI * 0x03613180) line 265 + 15 bytes
nsDocLoaderImpl::FireOnLocationChange(nsDocLoaderImpl * const 0x00e96af0,
nsIWebProgress * 0x00e96b04, nsIRequest * 0x00000000, nsIURI * 0x03613180) line 1140
nsDocShell::SetCurrentURI(nsDocShell * const 0x00eb10d0, nsIURI * 0x03613180)
line 1252
nsDocShell::OnNewURI(nsDocShell * const 0x00eb10d0, nsIURI * 0x03613180,
nsIChannel * 0x03615ed0, unsigned int 1) line 5487
nsDocShell::OnLoadingSite(nsDocShell * const 0x00eb10d0, nsIChannel *
0x03615ed0) line 5512
nsDocShell::CreateContentViewer(nsDocShell * const 0x00eb10d0, const char *
0x0012fc38, nsIRequest * 0x03615ed0, nsIStreamListener * * 0x0012fc88) line 4121
nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x00eb0e20,
const char * 0x0012fc38, int 0, nsIRequest * 0x03615ed0, nsIStreamListener * *
0x0012fc88, int * 0x0012fc24) line 107 + 33 bytes
nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x03615ed0, nsISupports *
0x00000000) line 357 + 93 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x03617ac0,
nsIRequest * 0x03615ed0, nsISupports * 0x00000000) line 227 + 16 bytes
nsHttpChannel::ProcessNormal() line 625 + 60 bytes
nsHttpChannel::ProcessResponse() line 527 + 8 bytes
nsHttpChannel::OnStartRequest(nsHttpChannel * const 0x03615ed4, nsIRequest *
0x03614cd4, nsISupports * 0x00000000) line 2824 + 11 bytes
nsOnStartRequestEvent::HandleEvent() line 161 + 53 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x035fbbd4) line 116
PL_HandleEvent(PLEvent * 0x035fbbd4) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00e6e6a0) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x01660470, unsigned int 49517, unsigned int 0,
long 15132320) line 1077 + 9 bytes
USER32! 77e71820()
0

Comment 6

[David Epstein]

nominating topembed. This is crashing whenever webProgLstnr is on and
OnLocationChange() is called back for a url load. I just get a request name in
OnLocationChange(). Before nsIWebProgressListener is frozen, we should decide
whether or not we're going to remove the request parameter as Rick suggests in
Comment #4. If it's not, the crash needs to be fixed by handling the passed request.

Comment 7

[Mitchell Stoltz (not reading bugmail)]

David, Rick,
   Do you think this crash might be exploitable (a buffer overrun?)

Comment 8

[rpotts (gone)]

No.  This is not a crash in the browser.  It is a crash in TestEmbed because the
request being passed out in OnLocationChange(...) is null.

TestEmbed does not check 'aRequest' before dereferencing it...

-- rick

Comment 9

[David Epstein]

Changing summary of the bug to address the source of the problem which is that
nsIRequest passed to OnLocationChange()is always null. Adding null check for
request handling in TestEmbed so it won't crash (thanks for pointing this out
Rick). We still need to decide what to do with the request parameter before
nsIWebProgressListener freezes.

Comment 10

[Darin Fisher]

mass reassign of various rpotts bugs to me

Comment 11

[Charles Rosendahl]

As I am now workingin this area, per edt, minusing.

Comment 12

[Vedran Miletic]

retargeting

Comment 13

[timeless]

we should decide what we want to do.

should 

/docshell/base/nsDocShell.cpp, line 1321 --
loader->FireOnLocationChange(webProgress, nsnull, aURI);

Create a nsLoadGroup and pass it?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I think rick was right.  We should just remove that aRequest arg, and do it
before we freeze this interface.... (nsIWebProgressListener etc).  Or we should
clearly document that in some cases a location change can happen without an
associated request and that the request may be null (if people are using
OnLocationChange for something interesting that uses aRequest).

Comment 15

[David Epstein]

What is happening with this bug? Is the request parameter going to be removed or
will the null condition be handled? I should point out that I was consistently
getting null when OnLocationChanged() was called (for a wide variety of urls). I
agree with removing the request parameter.

Comment 16

[Darin Fisher]

At this point, I do not think it would be wise to change this interface.  We
should instead document the fact that aRequest may be null in some cases, and
move on.  If we want a better interface, call it nsIWebProgressListener2 :-/

Comment 17

[Darin Fisher]

comment is here:

http://lxr.mozilla.org/mozilla/source/uriloader/base/nsIWebProgressListener.idl#214

marking WONTFIX