Closed
Bug 102376
Opened 24 years ago
Closed 24 years ago
crash when trying to view source of google.com - Trunk [@ nsParser::GetChannel]
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: ferdinandw+bmo, Assigned: harishd)
References
()
Details
(Keywords: crash, topcrash, Whiteboard: [fix in hand][critical to 0.9.5])
Crash Data
Attachments
(4 files)
1.01 KB,
patch
|
dp
:
review+
rpotts
:
superreview+
|
Details | Diff | Splinter Review |
2.18 KB,
patch
|
hjtoi-bugzilla
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
Details | Diff | Splinter Review | |
3.58 KB,
patch
|
hjtoi-bugzilla
:
review+
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4+)
Gecko/20010929
BuildID: 2001092909
When trying to view the source of Google, mozilla crashes.
This happens only on my cvs build (debug and opt), the 2903 nightly isn't crashing.
I've seen the view source command work exactly once on that page, other times it
crashed.
Syntax highlighting setting doesn't matter
Reproducible: Always
Steps to Reproduce:
1. Go to www.google.com
2. Click View Page Source in context menu
3. Try not to cry when mozilla crashes ;)
Actual Results: Crash in gkparser.dll, access violation
Expected Results: Get a chance to admire the view source window
msvc call stack of crash:
CallQueryInterface(nsIRequest * 0x03c1e908, nsIChannel * * 0x0012ecc8) line 270
+ 19 bytes
nsParser::GetChannel(nsParser * const 0x03d04ff0, nsIChannel * * 0x0012ecc8)
line 2703 + 22 bytes
nsObserverEntry::Notify(nsObserverEntry * const 0x00c8f8b8, nsIParserNode *
0x01017170, nsIParser * 0x03d04ff0, nsISupports * 0x03cb046c) line 1551 + 42 bytes
HTMLContentSink::NotifyTagObservers(HTMLContentSink * const 0x03d06578,
nsIParserNode * 0x01017170) line 3854 + 65 bytes
CViewSourceHTML::HandleToken(CViewSourceHTML * const 0x03ced940, CToken *
0x03d05248, nsIParser * 0x03d04ff0) line 1101 + 25 bytes
CViewSourceHTML::BuildModel(CViewSourceHTML * const 0x03ced940, nsIParser *
0x03d04ff0, nsITokenizer * 0x03c7c2e0, nsITokenObserver * 0x00000000,
nsIContentSink * 0x03d06578) line 611 + 23 bytes
nsParser::BuildModel() line 2022 + 34 bytes
nsParser::ResumeParse(int 1, int 1) line 1888 + 11 bytes
nsParser::ContinueParsing() line 1518 + 17 bytes
CSSLoaderImpl::Cleanup(URLKey & {...}, SheetLoadData * 0x03c43d90) line 736
CSSLoaderImpl::SheetComplete(nsICSSStyleSheet * 0x00000000, SheetLoadData *
0x03c43d90) line 843
CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * 0x03ce8828, SheetLoadData *
0x03c43d90, int & 1, nsICSSStyleSheet * & 0x0348b388) line 878
CSSLoaderImpl::DidLoadStyle(nsIStreamLoader * 0x03c99558, nsString * 0x03561608,
SheetLoadData * 0x03c43d90, unsigned int 0) line 913 + 27 bytes
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x03c43d90,
nsIStreamLoader * 0x03c99558, nsISupports * 0x00000000, unsigned int 0, unsigned
int 2842, const char * 0x03d5bfa0) line 670
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x03c9955c, nsIRequest *
0x03be4650, nsISupports * 0x00000000, unsigned int 0) line 136 + 81 bytes
nsFileChannel::OnStopRequest(nsFileChannel * const 0x03be4658, nsIRequest *
0x03c995d4, nsISupports * 0x00000000, unsigned int 0) line 481 + 41 bytes
nsOnStopRequestEvent::HandleEvent() line 177
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03d18fdc) line 80
PL_HandleEvent(PLEvent * 0x03d18fdc) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00c278d0) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x001b0242, unsigned int 49321, unsigned int 0,
long 12744912) line 1071 + 9 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x00d4a950) line 457
main1(int 1, char * * 0x003580f8, nsISupports * 0x00000000) line 1293 + 32 bytes
main(int 1, char * * 0x003580f8) line 1621 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e97d08()
Hmm, looks like the channel ( view source ) got released in the parser's blocked
state. CCing darin for input.
May be mRequest, in CParserContext, should be strong refd.
Status: NEW → ASSIGNED
Or CParserContext->mRequest should be nulled out in nsParser::OnStopRequest().
The draw back, however, would be if the channel is requested, by the sink or by
the charser observer, post OnStopRequest. Darin, any thoughts??
Comment 6•24 years ago
|
||
Per our discussion: I too think a strongref is the right thing here.
Comment 7•24 years ago
|
||
Bug 100566 may be a dupe of this.
Some more Talkbacks for 2001092908 Win2k:
TB36076720W
TB36076535Y
TB36076393H
Prior to my checkin to bug 96364 nsParserBundle, created by the parser, was
holding a strong reference to nsIChannel and therefore was available ( the
channel ) for the life time of the parser. However, after my checkin, to bug
96364, parser was not holding any reference to the channel. This caused the
channel to get released on an OnStopRequest and hence causing
CParserContext::mRequest ( which is the channel ) to dangle. By making
CParserContext to hold a strong reference to request we can guarantee the
availability of a channel for the life time of the parser.
Assignee | ||
Comment 11•24 years ago
|
||
Need r/sr ?
Comment 12•24 years ago
|
||
This is the same crash that happens on http://lautz.async.com.br/ and
http://www.helsinki.fi/ (which trips a crash on Mac too as per bug 100566).
Somebody _please_ s/sr this?
Keywords: patch
Comment 13•24 years ago
|
||
Comment on attachment 51466 [details] [diff] [review]
patch v1.0 [ CParserContext holds a strong reference to nsIRequest ]
Are we sure that this doesn't introduce any cycles - and thus leaks?
If so, then r/sr=rpotts@netscape.com
Attachment #51466 -
Flags: superreview+
Whiteboard: [fix in hand][Need r/sr] → [fix in hand][Need r/sr][critical to 0.9.5]
Reporter | ||
Comment 14•24 years ago
|
||
*** Bug 102465 has been marked as a duplicate of this bug. ***
Comment 15•24 years ago
|
||
Comment on attachment 51466 [details] [diff] [review]
patch v1.0 [ CParserContext holds a strong reference to nsIRequest ]
r=dp Making sure this causes no more leaks is good.
Attachment #51466 -
Flags: review+
Assignee | ||
Comment 16•24 years ago
|
||
With XPCOM_MEM_LEAK_LOG env set I did not see any leak ( as far as I can say ).
When viewing source, however, I do see nsCParserNode ( 4 nodes ) leaking but I
don't think it's anyway related to this change. Will work on that leak after I
come back from my vacation.
Whiteboard: [fix in hand][Need r/sr][critical to 0.9.5] → [fix in hand][critical to 0.9.5]
Assignee | ||
Comment 17•24 years ago
|
||
Rick: I'm concerned about the circularity too! However, I don't think the change
does anything different from what the parser bundle used to do. That is, the
parser bundle was in fact holding a strong reference to the channel and hence
had the potential to introduce circularity. Now, it's the parser context that's
holding a strong reference to the channel ( rather nsIRequest ). I'll run Purify
( after I'm back from my vacation! )and will make sure that this change does not
introduce anymore leaks. For now I think we should take the change and unblock
the tree.
Assignee | ||
Comment 18•24 years ago
|
||
Fix is in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•24 years ago
|
||
Ok, I have a better solution. Instead of the parser destructor releasing
mRequest why not release mRequest in nsParser::DidBuildModel(). After the call
to DidBuildModel() we shouldn't be needing channel anyway. Let me attach another
patch. And may be would be needing yet another r/sr :-/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Note: I haven't tested my last patch since I don't have a good tree and my modem
is too slow to check out a new tree. Could some one please test patch v1.1? Thanx.
If the new patch works as expected then I would need yet another r/sr. Sorry for
the trouble.
Comment on attachment 51493 [details] [diff] [review]
patch v1.1 [ release mRequest in nsParser::DidBuildModel() ]
Is it guaranteed that DidBuildModel() will be
called every time? If so, r=heikki. If it is not, then
we should NS_IF_RELEASE in the destructor as well.
Since the first fix is in, I think the improved fix
can wait until Harish gets back from vacation
Attachment #51493 -
Flags: review+
Hmm.. so how about changing mRequest to nsCOMPtr? In DidBuildModel() simply null
it out, and if it is not called no harm done (will be released in the
destructor). The nsCOMPtr also documents that it is an owning pointer.
Comment 25•24 years ago
|
||
Comment on attachment 51493 [details] [diff] [review]
patch v1.1 [ release mRequest in nsParser::DidBuildModel() ]
the v1.1 patch makes sense to me. the content-sink should only need to call
GetChannel before DidBuildModel, but why
not use a nsCOMPtr<nsIRequest>?
Comment 26•24 years ago
|
||
Comment on attachment 51493 [details] [diff] [review]
patch v1.1 [ release mRequest in nsParser::DidBuildModel() ]
sr=darin (with the change to using nsCOMPtr)
![]() |
||
Comment 27•24 years ago
|
||
*** Bug 102489 has been marked as a duplicate of this bug. ***
Comment 28•24 years ago
|
||
*** Bug 102373 has been marked as a duplicate of this bug. ***
Updated•24 years ago
|
OS: Windows 2000 → All
Comment 29•24 years ago
|
||
Comment 30•24 years ago
|
||
Comment on attachment 51618 [details] [diff] [review]
v1.1 same patch w/ some extra whitespace cleanup
r=heikki
I would prefer assigning nsnull to a pointer to clear
it (rather than 0) - that is one of our conventions.
Attachment #51618 -
Flags: review+
Comment 32•24 years ago
|
||
AFAIK assigning 0 to a COMPtr is the convention for mozilla, whereas nsnull is
used with regular pointers.
Comment 33•24 years ago
|
||
Adding topcrash keyword and Trunk [@ nsParser::GetChannel] to summary since
this is a topcrasher on the MozillaTrunk. Here's a summary of the crashes and a
stacktrace:
nsParser::GetChannel 58
BBID range: 36044915 - 36143158
Min/Max Seconds since last crash: 14 - 74256
Min/Max Runtime: 14 - 78342
Crash data range: 2001-09-29 to 2001-10-01
Build ID range: 2001092821 to 2001093021
Stack Trace:
nsParser::GetChannel
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 2705]
nsObserverEntry::Notify
[d:\builds\seamonkey\mozilla\htmlparser\src\nsDTDUtils.cpp line 1553]
HTMLContentSink::NotifyTagObservers
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp
line 3855]
CViewSourceHTML::HandleToken
[d:\builds\seamonkey\mozilla\htmlparser\src\nsViewSourceHTML.cpp line 1104]
CViewSourceHTML::BuildModel
[d:\builds\seamonkey\mozilla\htmlparser\src\nsViewSourceHTML.cpp line 612]
nsParser::BuildModel
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 2026]
nsParser::ResumeParse
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 1890]
nsParser::ContinueParsing
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 1520]
CSSLoaderImpl::Cleanup
[d:\builds\seamonkey\mozilla\content\html\style\src\nsCSSLoader.cpp line 736]
CSSLoaderImpl::SheetComplete
[d:\builds\seamonkey\mozilla\content\html\style\src\nsCSSLoader.cpp line 843]
CSSLoaderImpl::ParseSheet
[d:\builds\seamonkey\mozilla\content\html\style\src\nsCSSLoader.cpp line 878]
CSSLoaderImpl::DidLoadStyle
[d:\builds\seamonkey\mozilla\content\html\style\src\nsCSSLoader.cpp line 914]
SheetLoadData::OnStreamComplete
[d:\builds\seamonkey\mozilla\content\html\style\src\nsCSSLoader.cpp line 674]
nsStreamLoader::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamLoader.cpp line 138]
nsFileChannel::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\file\src\nsFileChannel.cpp line 484]
nsOnStopRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsRequestObserverProxy.cpp line 177]
PL_HandleEvent
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 591]
PL_ProcessPendingEvents
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 524]
_md_EventReceiverProc
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 1072]
KERNEL32.DLL + 0x248f7 (0xbff848f7)
0x00688b5e
0x00058f64
Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/htmlparser/src/nsParser.cpp
line : 2705
I won't bother posting all the user comments, since they all are about viewing
the page source, which we already know.
Keywords: topcrash
Summary: crash when trying to view source of google.com → crash when trying to view source of google.com - Trunk [@ nsParser::GetChannel]
Comment 34•24 years ago
|
||
*** Bug 102728 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 36•24 years ago
|
||
Fix in in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 37•24 years ago
|
||
Verified fixed on build ID 2001-10-22, No crashes, tested on Win2k, Linux and
MacOSX.
Setting bug to Verified and adding keyword vtrunk
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Updated•14 years ago
|
Crash Signature: [@ nsParser::GetChannel]
You need to log in
before you can comment on or make changes to this bug.
Description
•