Closed Bug 63529 Opened 25 years ago Closed 25 years ago

zero pointer deref in nsWebshell::OnStateChange

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: bernd_mozilla, Assigned: rpotts)

References

Details

Attachments

(2 files)

[patch] null ptr check
603 bytes, patch
Details | Diff | Splinter Review
Patch to ensure that mDocumentRequest is never null...
17.09 KB, patch
Details | Diff | Splinter Review
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsWebShell.cpp#905 channel can be zero. I just crashed there by switching between pages back and forward. I think there should be zero pointer check of the do_QueryInterface result. Win98 CVS20001221.
TB23406579Q, I crashed here reading news.
Keywords: crash
*** Bug 63523 has been marked as a duplicate of this bug. ***
Why is |channel| null, though? Maybe |aRequest| is null? Rick, is this patch okay or is there more going on here?
Severity: normal → critical
The channel should never be null in OnStateChange(...) I'm looking into why this is sometimes happening. It appears that the channel is sometimes null if the document has been cancelled... This is why you mostly see the crash when using back/forward... I believe that adding a null-pointer check in nsWebShell should be harmless (for now)...
Hrm...so should I check this in in the interim (r=rpotts?)?
I've checked in a null-pointer test at the top of nsWebShell::OnStateChange(...) This should stop the crash, but I'm leaving the bug open until I can figure out why the channel is null in the first place. -- rick
*** Bug 63630 has been marked as a duplicate of this bug. ***
*** Bug 63632 has been marked as a duplicate of this bug. ***
*** Bug 63623 has been marked as a duplicate of this bug. ***
*** Bug 63548 has been marked as a duplicate of this bug. ***
*** Bug 63637 has been marked as a duplicate of this bug. ***
adding mostfreq, this breaks the Bugzilla Helper. Adding dogfood keyword.
Keywords: dogfood, mostfreq
*** Bug 63638 has been marked as a duplicate of this bug. ***
All/All
OS: Windows 98 → All
Hardware: PC → All
Are people still seeing this crash - after the patch was checked in? If not, I'd like to remove the keywords and leave the bug open until I can investigate (and fix) why the channel is being passed in as null... -- rick
No, I'm not seeing this crash anymore.
*** Bug 63812 has been marked as a duplicate of this bug. ***
This is still the #1 topcrash in current builds. Perhaps |aRequest| is garbage rather than null? See ftp://ftp.mozilla.org/pub/data/crash-data/ for more details. In talkback it looks like the problem started in the evening builds on 2000-12-21, although this bug was filed earlier in the day. Any chance of looking through checkins to see what might have caused it?
Keywords: topcrash
the earliest crash is Build: 2000122021 CrashDate: 2000-12-21 fix for bug 15345 by rpotts 12/20/2000 15:18
*** Bug 63849 has been marked as a duplicate of this bug. ***
Is bug 62643 a dupe of this bug? I am wondering because the stacktrace for bug 62738 which was duped to 62643, and that of bug 63630 duped to this one are nearly identical. Though crash in bug 63630 disappeared after the patch here, 62643 still crashes. Moreover, I have started to get crash as in bug 65181 that gives me the same stacktrace.
Can't we get this null-check patch in, at least temporary? This is a topcrash, mostfreq and dogfood bug. So we can't just stay idle about it. r=hwaara on the patch if that matters...
Keywords: dogfood → nsdogfood
A slightly different null pointer check was already checked in (nsWebShell.cpp rev 1.508).
Target Milestone: --- → mozilla0.9
*** Bug 63634 has been marked as a duplicate of this bug. ***
anyone have a test case for this?
<Hixie> Is this still a crasher? Should it really be targetted at mozilla 0.9?
Removing keywords and moving out to a later milestone... I'm working on a "correct patch" which prevents the nsIRequest from ever being NULL :-)
Keywords: crash, mostfreq, nsdogfood, topcrash
Target Milestone: mozilla0.9 → mozilla0.9.1
I've just attached a patch that fixes this bug... It also fixes bug #55867 bug #26295 bug #63247... All of these are related bugs :-)
Blocks: 77421
These patches fix the following bugs: ------------------------------------- * bug #55867 which was opened to provide a real fix of the problem described in bug #27687. * bug #63529 which resulted from mDocumentRequest being null. A while ago I added a null pointer check in the DocShell/WebShell... But these patches make sure that mDocumentRequest is nenver null if the first place.. * bug #26295 which was the assertion that either the LoadGroup or DocLoader was still busy after being canceled. * bug #63247 which was tracking the need to change the arguments being passed on nsIHTTPEventSink::OnRedirect(...) These patches also affect the following bugs: ---------------------------------------------- * bug #76286 - I reworked the ASSERTION that waterson added to verify that mDocumentRequest was never null. I also reworked the way the mDocumentRequest is being set - to guarentee that it is NEVER null :-) * bug #59827 - I reworked the way that the STATE_TRANSFERRING notification is being fired * bug #27687 - I removed the band-aid patch in nsHTTPChannel since the real fix (in bug #55867) is provided. Here's a quick overview of the changes that these patches introduce... ---------------------------------------------------------------------- * nsLoadGroup has been modified to prevent new requests from being added while it is in the process of canceling its requests... * OnRedirect(...) has been reworked in nsHTTPChannel. The nsIHTTPEventSink::OnRedirect notification is now fired *before* the redirect occurs - and it is now cancelable (via a FAILURE return code) When a channel is redirected the status code passed out through OnStopRequest is now NS_BINDING_REDIRECTED * The signature of nsIHTTPEventSink::OnRedirect(...) has changed to be two channels - the old one and the new one... * nsDocLoader::OnStartRequest has been changed to only rely on the LOAD_DOCUMENT_URI flag to determine if a request represents the "document channel" or not... This is a more reliable check that seeing if the count == 1. It ensures that mDocumentRequest is never null during a page load... * nsDocLoader::DocLoaderIsEmpty() was changed to not pass a status code. In the current code, the argument was ignored - because it is useless :-) thanks, -- rick
patch checked in. -- rick
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: