1079141 - crash in nsScriptLoader::ReadyToExecuteScripts()

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Carsten Book [:Tomcat]]

This bug was filed from the Socorro interface and is 
report bp-7d9b2353-c813-4bce-8b9f-d14b82141007.
=============================================================

not really sure how to reproduce but will try

Crashing Thread
Frame 	Module 	Signature 	Source
0 	XUL 	nsScriptLoader::ReadyToExecuteScripts() 	content/base/src/nsScriptLoader.cpp
1 	XUL 	nsScriptLoader::ProcessPendingRequests() 	content/base/src/nsScriptLoader.cpp
2 	XUL 	nsScriptLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*) 	content/base/src/nsScriptLoader.cpp
3 	XUL 	nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) 	netwerk/base/src/nsStreamLoader.cpp
4 	XUL 	mozilla::net::HttpBaseChannel::DoNotifyListener() 	netwerk/protocol/http/HttpBaseChannel.cpp
5 	XUL 	mozilla::net::nsHttpChannel::HandleAsyncAbort() 	netwerk/protocol/http/HttpBaseChannel.h
6 	XUL 	nsRunnableMethodImpl<void (mozilla::net::nsHttpChannel::*)(), void, true>::Run() 	obj-firefox/x86_64/dist/include/nsThreadUtils.h
7 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
8 	XUL 	NS_ProcessPendingEvents(nsIThread*, unsigned int) 	xpcom/glue/nsThreadUtils.cpp
9 	XUL 	nsBaseAppShell::NativeEventCallback() 	widget/xpwidgets/nsBaseAppShell.cpp
10 	XUL 	nsAppShell::ProcessGeckoEvents(void*) 	widget/cocoa/nsAppShell.mm

Comment 1

[Boris Zbarsky [:bzbarsky]]

I "doc" a garbage pointer?  It can't actually be null, since we're null-checking it...

Comment 2

[(Away)]

The Windows crash reports are saying it's a few lines below that:
http://hg.mozilla.org/mozilla-central/annotate/0ed32d9a42d6/content/base/src/nsScriptLoader.cpp#l1250
 if (!mDocument->IsMasterDocument()) {

Gabor, is it normal for mDocument to be null?

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to David Major [:dmajor] (UTC+13) from comment #2)
> The Windows crash reports are saying it's a few lines below that:
> http://hg.mozilla.org/mozilla-central/annotate/0ed32d9a42d6/content/base/src/
> nsScriptLoader.cpp#l1250
>  if (!mDocument->IsMasterDocument()) {
> 
> Gabor, is it normal for mDocument to be null?

Yes, I think I should have guarded against mDocument being null here. Not sure how can it be null but apparently it can in some cases.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Most simply, couldn't the document just have gone away?  The scriptloader isn't holding a strong reference to it; it just gets its reference nulled out if the document is destroyed.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Most simply, couldn't the document just have gone away?

Yes, most likely that's the case. It's definitely a bug that I have not guarded against that since all the rest of the code does.

Comment 6

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=1e81fb10b217

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: mDocument null check in nsScriptLoader::ReadyToExecuteScripts. v1

I have no idea how to write a test for this though...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8504076 [details] [diff] [review]
mDocument null check in nsScriptLoader::ReadyToExecuteScripts. v1

r=me, I guess, but won't we just hit the null derefs in ProcessRequest?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8504076 [details] [diff] [review]
> mDocument null check in nsScriptLoader::ReadyToExecuteScripts. v1
> 
> r=me, I guess, but won't we just hit the null derefs in ProcessRequest?

hmmm... where? I only see mDocument derefs here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#1229 but that if branch is guarded. Am I missing something?

Comment 10

[Gabor Krizsanits (INACTIVE)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

> hmmm... where?

In ProcessRequest, not ProcessPendingRequests.  Like at http://hg.mozilla.org/mozilla-central/file/62f0b771583c/content/base/src/nsScriptLoader.cpp#l953 and http://hg.mozilla.org/mozilla-central/file/62f0b771583c/content/base/src/nsScriptLoader.cpp#l1026 called via AttemptAsyncScriptParse calling GetScriptGlobalObject

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> In ProcessRequest, not ProcessPendingRequests.

Right. Sorry, was focusing to much on the call stack it seems. So is it OK if we just return with error for those if mDocument is not set, or do you think we should do further investigation here? I mean, is it just a bug or is there an assumption I don't know about why mDocument cannot be null here? In that case I would at least expect some asserts...

Comment 13

[Boris Zbarsky [:bzbarsky]]

I see nothing preventing mDocument being null there, if it can be null in ReadyToExecuteScripts().

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Robert Kaiser]

[Tracking Requested - why for this release]:
This is now, after the grave gfx issues are solved, the largest remaining crash issue in 35.0b4, ranking as #2 with 4-5% of all crashes.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 8504076 [details] [diff] [review]
mDocument null check in nsScriptLoader::ReadyToExecuteScripts. v1

Approval Request Comment
[User impact if declined]:

This is currently the #2 top crash on beta and appears to be gone in aurora where this fix is included.

[Describe test coverage new/current, TBPL]:

AFAICT we're unable to reproduce this, but it's happening in beta but not in aurora where the fix is included which seems to indicate this does fix this bug.

[Risks and why]:

This adds a null check, very safe change.

[String/UUID change made/needed]: n/a

Comment 17

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-beta/rev/73145d9c6bde

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Can this be closed now?

Comment 19

[Gabor Krizsanits (INACTIVE)]

Yes, I think it should be closed.

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro stats [1] over the past week show no crashes in Firefox 35.0b8 and 36. Closing this.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsScriptLoader%3A%3AReadyToExecuteScripts%28%29