3.80 KB, text/plain
2.88 KB, patch
|Details | Diff | Splinter Review|
707 bytes, patch
|Details | Diff | Splinter Review|
i've tried to reproduce the crash using rc3 and user comments as a guide but was unsuccessful. below is the latest info from talkback: Count Offset Real Signature [ 4 nsCharTraits<unsigned short>::length 00a06a8e - nsCharTraits<unsigned short>::length ] [ 1 nsCharTraits<unsigned short>::length e1d36f92 - nsCharTraits<unsigned short>::length ] Crash date range: 2002-05-24 to 2002-05-27 Min/Max Seconds since last crash: 2010 - 84541 Min/Max Runtime: 7428 - 84541 Keyword List : Count Platform List 4 Windows NT 5.1 build 2600 1 Windows NT 5.0 build 2195 Count Build Id List 5 2002052308 No of Unique Users 4 Stack trace(Frame) nsCharTraits<unsigned short>::length [..\..\dist/include/string\nsCharTraits.h line 201] nsDependentString::Rebind [..\..\dist/include/string\nsDependentString.h line 60] nsAString::do_AppendFromElementPtr [d:\builds\seamonkey\mozilla\string\src\nsAString.cpp line 360] nsAutoString::nsAutoString [d:\builds\seamonkey\mozilla\string\obsolete\nsString2.cpp line 1202] nsMetaCharsetObserver::Notify [d:\builds\seamonkey\mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp line 280] nsMetaCharsetObserver::Notify [d:\builds\seamonkey\mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp line 165] nsObserverEntry::Notify [d:\builds\seamonkey\mozilla\htmlparser\src\nsDTDUtils.cpp line 1578] HTMLContentSink::NotifyTagObservers [d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp line 3953] CNavDTD::WillHandleStartTag [d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp line 1400] CNavDTD::HandleStartToken [d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp line 1649] CNavDTD::HandleToken [d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp line 908] CNavDTD::BuildModel [d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp line 521] nsParser::BuildModel [d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 1869] nsParser::ResumeParse [d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 1733] nsParser::OnDataAvailable [d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp line 2369] nsDocumentOpenInfo::OnDataAvailable [d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp line 245] nsStreamListenerTee::OnDataAvailable [d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerTee.cpp line 98] nsHttpChannel::OnDataAvailable [d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp line 2965] nsOnDataAvailableEvent::HandleEvent [d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerProxy.cpp line 203] PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 597] PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 530] _md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c line 1078] USER32.dll + 0x3c076 (0x77d7c076) USER32.dll + 0x3c076 (0x77d7c076) _except_handler3() kernel32.dll + 0x3bb86 (0x77e9bb86) (6693358) URL: http://www.nytimes.com (6693358) Comments: I had several tabs open and I clicked on a new link inside one of the tabbed windows to load a new page. (6685112) URL: freshmeat.net (6685112) Comments: Opening URL from freshmeat.net causes crash
Assignee: jaggernaut → yokoyama
Component: String → Internationalization
QA Contact: scc → ruixu
Summary: M1RC3 crash [@ nsCharTraits<unsigned ] → M1RC3 crash [@ nsCharTraits<unsigned short>::length]
nsCharTraits<PRunichar>::length(ptr) can crash for a few reasons. One is that |ptr| is |0|, another is that |ptr| points to memory we're not allowed to read. In this case I suspect it's the latter that's happening. 279 nsAutoString contentPart1(contentValue+10); // after "text/html;" Except the test above is for "text/html" so +10 may put it right after the null-terminator. I suspect changing it to +9 will fix this. Patch coming up.
Created attachment 85389 [details] [diff] [review] Fix Untested patch, the second chunk is the fix, the first chunk is a simplification of the test.
Nominating for nsbeta1.
while i don't think this patch is correct, i don't believe the original code was either (ignoring crashes). offhand, i've seen this crumy code before, and i'm pretty sure i've complained about it before. fwiw http://www.ietf.org/rfc/rfc2045.txt specifies content type rules http:// www.ietf.org/rfc/rfc2234.txt describes abnf section 3.1 covers 'LINEAR WHITE SPACE:' so what we should do is RFind, and then verify that the preceding character is a space, semicolon or tab.
examples of inputs that the current and proposed code don't handle correctly: 'text/html; charset=LATIN-1; foocharset=US-ASCII' in fact, we should probably tokenize around ';' or scan forward for it.
This patch fixes the crash, and throws in a simplification of code for free. No, it doesn't make Mozilla perfect, but I never said it would do that. Yes, we should probably be a little smarter in the parsing, but right now the code works with what the webpages provide, since there is no site out there that'll have a "foocharset" (because there's no point in putting that in). File another bug on the parsing if you must, leave this bug to address this crash.
Lots of instances of this crash showing up for nsCharTraits::length in each of the mozilla release candidates, and continuing on the branch and trunk. If this patch is good, then let's move forward on landing it to trunk.
Keywords: mozilla1.0.1, patch
Comment on attachment 85389 [details] [diff] [review] Fix sr=blake
Attachment #85389 - Flags: superreview+
Comment on attachment 85389 [details] [diff] [review] Fix r=dbaron. If you want to be pedantic, the following is incorrect: >+ Substring(httpEquivValue, >+ httpEquivValue+contenttype.Length()).Equals(contenttype, >+ nsCaseInsensitiveStringComparator()) >+ && >+ Substring(contentValue, >+ contentValue+texthtml.Length()).Equals(texthtml, >+ nsCaseInsensitiveStringComparator()) Since you don't know the lengths of contentValue or httpEquivValue, but you'll probably get away without crashing. It's worth testing with short strings, though.
Yeah, I saw that when I read the code. It's ugly, but it works as is because contentValue and httpEquivValue are null terminated, which'll guarantee we won't try to read beyond the null (since we know that the strings we're comparing against don't have embedded nulls).
Comment on attachment 88592 [details] [diff] [review] Suggested fix for branch Carrying over r and sr since this is exactly the second hunk of the trunk patch
Taking bug. Adding nsbeta1 and adt1.0.1, I think we really want the small fix on the branch, if it indeed fixes this crash (I'm pretty sure it does). Since we don't have a testcase, let's see if the fix on the trunk removes this stack from trunk talkback data. John, can you keep an eye on that?
Assignee: yokoyama → jaggernaut
Keywords: adt1.0.1, nsbeta1
And marking fixed since I checked it in on the trunk today.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: [adt2 RTM]
Target Milestone: --- → mozilla1.0.1
Change QA contact to firstname.lastname@example.org per comment #15, cause there is not way for me to verify it.
QA Contact: ylong → jrgm
adding adt1.0.1+. Please get drivers approval and land before the 6/25 branch build.
Keywords: adt1.0.1 → adt1.0.1+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Verified fixed for the trunk. No crashes reported for trunk builds with this stack trace since 06/18, after 29 crashes reported 06/09 to 06/18 (3 per day).
Status: RESOLVED → VERIFIED
*** Bug 154664 has been marked as a duplicate of this bug. ***
What is the signature to look at for the branch builds in talkback? nsCharTraits<unsigned short>::length ?
talkback gets a little baffled by C++ template syntax. I use just 'nsCharTraits' to get a match. There are sporadic cases of a crash ending in nsCharTraits::<unsigned short>length on the trunk, although not apparently the same crash as this one (the bug is really with the callers, not 'length'). There weren't any crashes on the branch in 'length' until a couple of days ago when junruh had 8 crashes while doing something with certificates (according to the comments). I don't know if a bug was opened for those crashes, but it would be a different bug than this one. As for this bug, per talkback and the checkin log (188.8.131.52 of nsMetaCharsetObserver.cpp), verified for the branch.
Keywords: fixed1.0.1 → verified1.0.1
Doh, meant to cc: junruh on the previous comment just to ask if there was a bug on a file for a crash in |nsCharTraits::<unsigned short>length| when working with certificates (since you had those talkback crashes in the past few days).
I believe that bug 152382 causes the crash in the cert manager.
Most recent build: 2003011305, reopening. This is currently #19 on the trunk topcrash list. nsCharTraits<unsigned 9 Crash data range: 2003-01-12 to 2003-01-13 Build ID range: 2003011204 to 2003011305 Stack Trace: nsCharTraits<unsigned short>::length [../../dist/include/string\nsCharTraits.h line 200] nsDependentString::Rebind [../../dist/include/string\nsDependentString.h line 60] nsAString::do_AssignFromElementPtr [c:/builds/seamonkey/mozilla/string/src/nsAString.cpp line 303] nsMIMEInfoImpl::SetDefaultDescription [c:/builds/seamonkey/mozilla/netwerk/mime/src/nsMIMEInfoImpl.cpp line 295] XPTC_InvokeByIndex [c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp line 102] XPCWrappedNative::CallMethod [c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp line 2025] XPC_WN_GetterSetter [c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp line 1317] js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 841] js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 932] js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 2648] js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 857] js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 932] JS_CallFunctionValue [c:/builds/seamonkey/mozilla/js/src/jsapi.c line 3433] nsJSContext::CallEventHandler [c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp line 1043] GlobalWindowImpl::RunTimeout [c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp line 4749] GlobalWindowImpl::TimerCallback [c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp line 5105] nsTimerImpl::Fire [c:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp line 383] nsAppShell::Run [c:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp line 176] nsAppShellService::Run [c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp line 471] main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1559] main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1907] WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1927] WinMainCRTStartup() kernel32.dll + 0x214c7 (0x77e814c7) Source File : ../../dist/include/string/nsCharTraits.h line : 200
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Closing this bug again as fixed. Jan, the stack looks a bit different. I think we should log a new bug if we continue to see these crashes.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago → 16 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsCharTraits<unsigned short>::length]
You need to log in before you can comment on or make changes to this bug.