Closed Bug 941888 Opened 6 years ago Closed 6 years ago

crash in nsIDocument::IsRootDisplayDocument()

Categories

(Core :: DOM: Core & HTML, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 - fixed
firefox28 --- fixed

People

(Reporter: mats, Assigned: smaug)

References

Details

(Keywords: crash, Whiteboard: [qa-])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-744e403c-a38a-4757-b37f-0d0192131121.
=============================================================

nsIDocument::IsRootDisplayDocument()
nsXULElement::GetWindowWidget()
nsXULElement::SetChromeMargins(nsAttrValue const *)
nsXULElement::AfterSetAttr(int,nsIAtom *,nsAttrValue const *,bool)
mozilla::dom::Element::SetAttrAndNotify(int,nsIAtom *,nsIAtom *,nsAttrValue const &,nsAttrValue &,unsigned char,bool,bool,bool)
mozilla::dom::Element::SetAttr(int,nsIAtom *,nsIAtom *,nsAString_internal const &,bool)
nsXMLContentSink::AddAttributes(wchar_t const * *,nsIContent *)
nsXMLContentSink::HandleStartElement(wchar_t const *,wchar_t const * *,unsigned int,int,unsigned int,bool)
nsXMLContentSink::HandleStartElement(wchar_t const *,wchar_t const * *,unsigned int,int,unsigned int)
nsExpatDriver::HandleStartElement(wchar_t const *,wchar_t const * *)
Driver_HandleStartElement
doContent
contentProcessor
doProlog
prologProcessor
prologInitProcessor
MOZ_XML_Parse
nsExpatDriver::ParseBuffer(wchar_t const *,unsigned int,bool,unsigned int *)
nsExpatDriver::ConsumeToken(nsScanner &,bool &)
nsParser::Tokenize(bool)
nsParser::ResumeParse(bool,bool,bool)
nsParser::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int)
nsXMLHttpRequest::StreamReaderFunc(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *)
nsPipeInputStream::ReadSegments(tag_nsresult (*)(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *),void *,unsigned int,unsigned int *)
nsXMLHttpRequest::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int)
nsStreamListenerWrapper::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int)
nsJARChannel::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int)
nsInputStreamPump::OnStateTransfer()
nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *)
nsInputStreamReadyEvent::Run()
nsThread::ProcessNextEvent(bool,bool *)
It's low volume but it seems to have become much more frequent lately.
Here's the breakdown of incidents over the past 28 days (all on Windows):
Firefox 28.0a1 	92.37 %	109
Firefox 27.0a1 	6.78 %	8
Firefox 7.0.1 	0.85 %	1
Perhaps it has become more common since Australis uses svg images, 
chrome://browser/skin/tabbrowser/tab-selected-start.svg and similar.
Assignee: nobody → bugs
Bug 515880 removed null checks.
Blocks: 515880
Perhaps we simply need to null-check 'doc' here:
http://hg.mozilla.org/mozilla-central/annotate/f2adb62d07eb/content/xul/content/src/nsXULElement.cpp#l1777
(all the 27/28 crashes have crash address 0xbc)
Attached patch null checkSplinter Review
This should do it. Based on crash-stat this is null+offset crash.
Attachment #8336394 - Flags: review?(matspal)
FYI, one of the crash reports has a user comment that suggests a way to reproduce it --
maybe it could be used to construct a crash tests?
bp-a1da0089-879f-411e-800d-a962a2131116
Comment on attachment 8336394 [details] [diff] [review]
null check

r=mats
Attachment #8336394 - Flags: review?(matspal) → review+
Keywords: checkin-needed
Comment on attachment 8336394 [details] [diff] [review]
null check

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 515880
User impact if declined: crash (safe)
Testing completed (on m-c, etc.): not landed yet
Risk to taking this patch (and alternatives if risky): adds null-check, zero risk
String or IDL/UUID changes made by this patch: none

I think we want this on Aurora(27) too.  Probably not needed for Beta(26)
or older though, since there were no incidents reported there.
Attachment #8336394 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e9e744a809df
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Mats Palmgren (:mats) from comment #8)
> Comment on attachment 8336394 [details] [diff] [review]
> null check
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 515880
> User impact if declined: crash (safe)
> Testing completed (on m-c, etc.): not landed yet
> Risk to taking this patch (and alternatives if risky): adds null-check, zero
> risk
> String or IDL/UUID changes made by this patch: none
> 
> I think we want this on Aurora(27) too.  Probably not needed for Beta(26)
> or older though, since there were no incidents reported there.

Don't see any crash reports on Fx 27 : https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=7&range_unit=days&date=2013-11-24&signature=nsIDocument%3A%3AIsRootDisplayDocument%28%29&version=Firefox%3A28.0a1

Not sure if we really need this on aurora. Can you help in case I am missing something ?
Flags: needinfo?(matspal)
You have "version=Firefox%3A28.0a1" in that query.  There are only a few crashes on 27,
but since the fix is truly trivial I figured we should take it.
Flags: needinfo?(matspal)
Attachment #8336394 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Mats Palmgren (:mats) from comment #12)
> You have "version=Firefox%3A28.0a1" in that query.  There are only a few
> crashes on 27,
> but since the fix is truly trivial I figured we should take it.

Thanks :mats, no need to track in that case, although approving for uplift given the trivial patch.
If this needs extra QA testing please remove the [qa-] whiteboard tag and add the verifyme keyword.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.