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)

x86
All
defect
Not set
critical

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)

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??
Keywords: crash
Apparently, my fix to bug 96364 has exposed this problem.
Can we use nsWeakPtr to avoid dangling?
Per our discussion: I too think a strongref is the right thing here.
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.
Need r/sr ?
Whiteboard: [fix in hand][Need r/sr]
Need r/sr ?
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 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]
*** Bug 102465 has been marked as a duplicate of this bug. ***
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+
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]
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.
Fix is in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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 → ---
Status: REOPENED → ASSIGNED
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.
is bug 102489 somehow related to this?
Keywords: mozilla0.9.5
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 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 on attachment 51493 [details] [diff] [review] patch v1.1 [ release mRequest in nsParser::DidBuildModel() ] sr=darin (with the change to using nsCOMPtr)
*** Bug 102489 has been marked as a duplicate of this bug. ***
*** Bug 102373 has been marked as a duplicate of this bug. ***
OS: Windows 2000 → All
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+
AFAIK assigning 0 to a COMPtr is the convention for mozilla, whereas nsnull is used with regular pointers.
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]
*** Bug 102728 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla0.9.5
--> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Fix in in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
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
Crash Signature: [@ nsParser::GetChannel]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: