Closed Bug 1079141 Opened 5 years ago Closed 5 years ago

crash in nsScriptLoader::ReadyToExecuteScripts()

Categories

(Core :: General, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + verified
firefox36 --- verified

People

(Reporter: cbook, Assigned: gkrizsanits)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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
I "doc" a garbage pointer?  It can't actually be null, since we're null-checking it...
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?
Blocks: 877072
Flags: needinfo?(gkrizsanits)
(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.
Flags: needinfo?(gkrizsanits)
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.
(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.
I have no idea how to write a test for this though...
Assignee: nobody → gkrizsanits
Status: NEW → ASSIGNED
Attachment #8504076 - Flags: review?(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?
Attachment #8504076 - Flags: review?(bzbarsky) → review+
(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?
> 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
(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...
Keywords: leave-open
I see nothing preventing mDocument being null there, if it can be null in ReadyToExecuteScripts().
[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 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
Attachment #8504076 - Flags: approval-mozilla-beta?
Attachment #8504076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can this be closed now?
Flags: needinfo?(gkrizsanits)
Keywords: leave-open
Target Milestone: --- → mozilla36
Yes, I think it should be closed.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gkrizsanits)
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.