941888 - crash in nsIDocument::IsRootDisplayDocument()

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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 *)

Comment 1

[Mats Palmgren (inactive)]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Perhaps it has become more common since Australis uses svg images, 
chrome://browser/skin/tabbrowser/tab-selected-start.svg and similar.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Bug 515880 removed null checks.

Comment 4

[Mats Palmgren (inactive)]

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)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: null check

This should do it. Based on crash-stat this is null+offset crash.

Comment 6

[Mats Palmgren (inactive)]

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 7

[Mats Palmgren (inactive)]

Comment on attachment 8336394 [details] [diff] [review]
null check

r=mats

Comment 8

[Mats Palmgren (inactive)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9e744a809df

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9e744a809df

Comment 11

[bhavana bajaj [:bajaj]]

(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 ?

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

[bhavana bajaj [:bajaj]]

(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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/644e577111a0

Comment 15

[u279076]

If this needs extra QA testing please remove the [qa-] whiteboard tag and add the verifyme keyword.