Request passed to OnLocationChange() is always null

RESOLVED WONTFIX

Status

()

Core
Embedding: APIs
RESOLVED WONTFIX
17 years ago
14 years ago

People

(Reporter: David Epstein, Assigned: Darin Fisher)

Tracking

({embed, topembed-})

Trunk
x86
Windows NT
embed, topembed-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

(Reporter)

Description

17 years ago
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

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

Comment 2

17 years ago
changed qa contact to depstein.
QA Contact: mdunn → depstein

Comment 3

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

Updated

17 years ago
Target Milestone: --- → mozilla0.9.7

Updated

16 years ago
Target Milestone: mozilla0.9.7 → mozilla1.0.1

Comment 4

16 years ago
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
(Reporter)

Comment 5

16 years ago
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
(Reporter)

Updated

16 years ago
Keywords: crash

Updated

16 years ago
Keywords: topembed → embed, topembed-
(Reporter)

Comment 6

16 years ago
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.
Severity: major → critical
Keywords: topembed- → topembed

Updated

16 years ago
Keywords: topembed → topembed+
David, Rick,
   Do you think this crash might be exploitable (a buffer overrun?)

Comment 8

16 years ago
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
(Reporter)

Comment 9

16 years ago
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.
Keywords: crash
Summary: Calling a request in OnLocationChange() crashes embedding app → Request passed to OnLocationChange() is always null
(Reporter)

Updated

16 years ago
Blocks: 99639
(Reporter)

Updated

16 years ago
QA Contact: depstein → carosendahl
(Assignee)

Comment 10

15 years ago
mass reassign of various rpotts bugs to me
Assignee: rpotts → darin

Comment 11

15 years ago
As I am now workingin this area, per edt, minusing.
Keywords: topembed+ → topembed-

Comment 12

15 years ago
retargeting
Target Milestone: mozilla1.0.1 → Future

Comment 13

15 years ago
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?
Severity: critical → normal
Target Milestone: Future → ---
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).
(Reporter)

Comment 15

14 years ago
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.
(Assignee)

Comment 16

14 years ago
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 :-/
(Assignee)

Comment 17

14 years ago
comment is here:

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

marking WONTFIX
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.