Closed Bug 86521 Opened 23 years ago Closed 20 years ago

Request passed to OnLocationChange() is always null

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Windows NT
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: depman1, Assigned: darin.moz)

References

()

Details

(Keywords: embed, topembed-)

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));
The reason this is crashing is because the request passed out through OnLocationChanged(...) is *always* NULL.
changed qa contact to depstein.
QA Contact: mdunn → depstein
is the request always being null a bug, or should we remove the arg altogether from OnLocationChange()?
Target Milestone: --- → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla1.0.1
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
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
Keywords: crash
Keywords: topembedembed, topembed-
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
Keywords: topembedtopembed+
David, Rick, Do you think this crash might be exploitable (a buffer overrun?)
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
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
Blocks: 99639
QA Contact: depstein → carosendahl
mass reassign of various rpotts bugs to me
Assignee: rpotts → darin
As I am now workingin this area, per edt, minusing.
Keywords: topembed+topembed-
retargeting
Target Milestone: mozilla1.0.1 → Future
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).
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.
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 :-/
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.