Closed Bug 912514 Opened 6 years ago Closed 6 years ago

crash spike in nsStandardURL::Release()

Categories

(Core :: Networking, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox26 + fixed

People

(Reporter: kairo, Assigned: seth)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-988403e7-20af-4288-8dbf-8d18a2130904.
=============================================================

Full Stack Trace:
0 	xul.dll 	nsStandardURL::Release() 	netwerk/base/src/nsStandardURL.cpp
1 	xul.dll 	mozilla::image::ImageResource::~ImageResource() 	
2 	xul.dll 	mozilla::image::RasterImage::`vector deleting destructor'(unsigned int) 	
3 	xul.dll 	mozilla::image::RasterImage::Release() 	image/src/RasterImage.cpp
4 	xul.dll 	mozilla::image::RasterImage::DecodePool::DecodeJob::~DecodeJob() 	
5 	xul.dll 	mozilla::image::RasterImage::DecodePool::DecodeJob::`scalar deleting destructor'(unsigned int) 	
6 	xul.dll 	nsRunnable::Release() 	obj-firefox/xpcom/build/nsThreadUtils.cpp
7 	xul.dll 	nsRefPtr<nsIRunnable>::~nsRefPtr<nsIRunnable>() 	obj-firefox/dist/include/nsAutoPtr.h
8 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
9 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
10 	xul.dll 	nsThread::ThreadFunc(void *) 	xpcom/threads/nsThread.cpp
11 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
12 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
13 	msvcr100.dll 	_callthreadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
14 	msvcr100.dll 	_threadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
15 	kernel32.dll 	BaseThreadInitThunk 	
16 	ntdll.dll 	__RtlUserThreadStart 	
17 	ntdll.dll 	_RtlUserThreadStart 	

This started happening with the 2013-08-31 26.0a1 Nightly build and is now #3 on trunk for builds from the last 3 days.

More crashes at https://crash-stats.mozilla.com/report/list?signature=nsStandardURL%3A%3ARelease%28%29
Keywords: topcrash
Flags: needinfo?(seth)
The fact that nsStandardURL is being released off the main thread worries me, since it does not appear to be threadsafe (ie. just has NS_IMPL_ISUPPORTS).
Although if OnDataAvailable is called off the main thread now, that means that we're now creating images off the main thread as well: http://mxr.mozilla.org/mozilla-central/source/image/src/imgRequest.cpp#758 . Interesting.
nsiuri is definitely not thread happy. (I've got many hacks to work around that)
I am working on this right now. Will have a patch shortly.

@jdm: To my knowledge OnDataAvailable is not called OMT yet. Steve Workman is working on a patch for that, though.
Assignee: nobody → seth
Flags: needinfo?(seth)
This should resolve the issue, and probably also bug 855330.
Attachment #800419 - Flags: review?(josh)
Blocks: 855330
Comment on attachment 800419 [details] [diff] [review]
Don't release nsIURIs off-main-thread.

Unless I'm mistaken, ImageResource should contain an nsMainThreadPtrHandle member, not nsMainThreadPtrHolder. The handle should contain an nsMainThreadPtrHolder value, however.
Attachment #800419 - Flags: review?(josh) → review-
Ahh dang, totally right. Will upload a revised patch in just a sec.
Just a sec turned out to be longer than I hoped because clobbers and such were involved. At any rather, this new patch should use nsMainThreadPtrHandle/Holder correctly.
Attachment #801020 - Flags: review?(josh)
Attachment #800419 - Attachment is obsolete: true
Comment on attachment 801020 [details] [diff] [review]
Don't release nsIURIs off-main-thread.

Review of attachment 801020 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/Image.h
@@ +155,5 @@
>    virtual void SetHasError() MOZ_OVERRIDE { mError = true; }
>  
>    /*
>     * Returns a non-AddRefed pointer to the URI associated with this image.
> +   * Illegal to use off-main-thread except in debug builds.

Why do we want this behaviour difference in debug builds?
(In reply to Josh Matthews [:jdm] from comment #9)
> Why do we want this behaviour difference in debug builds?

Because it's frequently useful to inspect this from GDB or in logging output when tracking down a problem, but making this actually safe (i.e., making it something that I'd be comfortable with release code depending on) would involve copying the URI into a string or some other threadsafe data structure, which is overhead we don't really gain anything from in release builds.
Note, in case it's not clear, that nsMainThreadPtrHandle/Holder have asserts to enforce this. It's not just enforced by hoping that people read the comment.
I feel very uncomfortable with code that aborts in more cases in release builds than debug builds. Could you be more specific about why having it not be strict in debug builds is useful? Why isn't this just something that can be toggled by somebody who cares?
This rebases the patch and addresses Josh's concern by only disabling strict mode if DEBUG_imagelib is #define'd, rather than in all debug builds.
Attachment #803310 - Flags: review?(josh)
Attachment #801020 - Attachment is obsolete: true
Attachment #801020 - Flags: review?(josh)
Attachment #803310 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/1c9fc8a6e707
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 803310 [details] [diff] [review]
Don't release nsIURIs off-main-thread.

Review of attachment 803310 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/Image.h
@@ +156,5 @@
>    virtual void SetHasError() MOZ_OVERRIDE { mError = true; }
>  
>    /*
>     * Returns a non-AddRefed pointer to the URI associated with this image.
> +   * Illegal to use off-main-thread.

Why not assert this?
(In reply to :Ms2ger from comment #16)
> Why not assert this?

I probably should have, but all this code has since been replaced anyway.
You need to log in before you can comment on or make changes to this bug.