Closed Bug 1382380 Opened 7 years ago Closed 7 years ago

assertion failure: ScriptLoadHandler not thread-safe

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mayhemer, Assigned: schien)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

(not sure I haven't reported this earlier already...)

today m-c, win, happened during shutdown:

the thread where the release happens doesn't have a name (a naming should be added!)

the mThread member seems to point to the main thread.

>	mozglue.dll!MOZ_CrashOOL(0x193ba528, 43, 0x1b581fb8) Line 33	C++
 	xul.dll!nsAutoOwningThread::AssertCurrentThreadOwnsMe(0x1b581fb8) Line 45	C++
 	xul.dll!nsAutoOwningThread::AssertOwnership<34>(...) Line 70	C++
 	xul.dll!mozilla::dom::ScriptLoadHandler::Release() Line 41	C++
 	xul.dll!nsCOMPtr<nsIIncrementalStreamLoaderObserver>::~nsCOMPtr<nsIIncrementalStreamLoaderObserver>() Line 406	C++
 	xul.dll!nsIncrementalStreamLoader::~nsIncrementalStreamLoader() Line 21	C++
 	xul.dll!nsIncrementalStreamLoader::`scalar deleting destructor'()	C++
 	xul.dll!nsIncrementalStreamLoader::Release() Line 45	C++
 	xul.dll!nsCOMPtr<nsIStreamListener>::~nsCOMPtr<nsIStreamListener>() Line 406	C++
 	xul.dll!mozilla::net::HttpBaseChannel::~HttpBaseChannel() Line 239	C++
 	xul.dll!mozilla::net::HttpChannelChild::~HttpChannelChild() Line 207	C++
 	xul.dll!mozilla::net::HttpChannelChild::`scalar deleting destructor'()	C++
 	xul.dll!mozilla::net::HttpChannelChild::Release() Line 249	C++
 	xul.dll!mozilla::RefPtrTraits<mozilla::net::HttpChannelChild>::Release(0x29d4d000) Line 41	C++
 	xul.dll!RefPtr<mozilla::net::HttpChannelChild>::ConstRemovingRefPtrTraits<mozilla::net::HttpChannelChild>::Release(0x29d4d000) Line 395	C++
 	xul.dll!RefPtr<mozilla::net::HttpChannelChild>::~RefPtr<mozilla::net::HttpChannelChild>() Line 78	C++
 	xul.dll!mozilla::net::HttpBackgroundChannelChild::ActorDestroy(Deletion) Line 470	C++
 	xul.dll!mozilla::net::PHttpBackgroundChannelChild::DestroySubtree(Deletion) Line 418	C++
 	xul.dll!mozilla::net::PHttpBackgroundChannelChild::OnMessageReceived({...}) Line 371	C++
 	xul.dll!mozilla::ipc::PBackgroundChild::OnMessageReceived({...}) Line 1608	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchAsyncMessage({...}) Line 2093	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW({...}) Line 2021	C++
 	xul.dll!mozilla::ipc::MessageChannel::RunMessage({...}) Line 1889	C++
 	xul.dll!mozilla::ipc::MessageChannel::MessageTask::Run() Line 1922	C++
 	xul.dll!nsThread::ProcessNextEvent(true, 0x1e6ff171) Line 1447	C++
 	xul.dll!NS_ProcessNextEvent(0x01784240, true) Line 489	C++
 	xul.dll!mozilla::net::nsSocketTransportService::Run() Line 976	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x1e6ffa45) Line 1447	C++
 	xul.dll!NS_ProcessNextEvent(0x01784240, false) Line 489	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(0x0313e480) Line 339	C++
 	xul.dll!MessageLoop::RunInternal() Line 321	C++
 	xul.dll!MessageLoop::RunHandler() Line 314	C++
 	xul.dll!MessageLoop::Run() Line 294	C++
 	xul.dll!nsThread::ThreadFunc(0x00cfda14) Line 508	C++
 	nss3.dll!_PR_NativeRunThread(0x01751920) Line 397	C
 	nss3.dll!pr_root(0x01751920) Line 95	C
 	ucrtbase.dll!__set_app_type()	Unknown
 	kernel32.dll!@BaseThreadInitThunk@12()	Unknown
 	mozglue.dll!patched_BaseThreadInitThunk(0, 0x758ae840, 0x02f2b7f8) Line 816	C++
 	ntdll.dll!__RtlUserThreadStart()	Unknown
 	ntdll.dll!__RtlUserThreadStart@8()	Unknown
Flags: needinfo?
And I can provide a log - timestamp,sync,nsHttp:5,cache2:5,RequestContext:5
Flags: needinfo?
Flags: needinfo?(schien)
From the crash stack, It was happened on socket transport thread.
> xul.dll!mozilla::net::nsSocketTransportService::Run() Line 976	C++

The main logic flaw is that we don't know whether the listener supports OMT or not. One thing we can do is to ensure mListener is released on main thread if RetargetDeliveryTo is never called.
Flags: needinfo?(schien)
Assignee: nobody → schien
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #2)
> From the crash stack, It was happened on socket transport thread.
> > xul.dll!mozilla::net::nsSocketTransportService::Run() Line 976	C++
> 
> The main logic flaw is that we don't know whether the listener supports OMT
> or not. One thing we can do is to ensure mListener is released on main
> thread if RetargetDeliveryTo is never called.

Hmm... maybe the simplest solution is to always release mListener on the main thread (add it to the list in HttpBaseChannel::ReleaseMainThreadOnlyReferences()).  We could save small amount of main thread time when we know it's safe to release on any thread, tho.  But only if we have that information available 100% reliably - if, as you said, we called RetargetDeliveryTo with success _and_ the mListener has not changed since that or has not been added a new listener to the chain since that time.  That can IMO never be known for 100%.
(In reply to Honza Bambas (:mayhemer) from comment #3)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #2)
> > From the crash stack, It was happened on socket transport thread.
> > > xul.dll!mozilla::net::nsSocketTransportService::Run() Line 976	C++
> > 
> > The main logic flaw is that we don't know whether the listener supports OMT
> > or not. One thing we can do is to ensure mListener is released on main
> > thread if RetargetDeliveryTo is never called.
> 
> Hmm... maybe the simplest solution is to always release mListener on the
> main thread (add it to the list in
> HttpBaseChannel::ReleaseMainThreadOnlyReferences()).  We could save small
> amount of main thread time when we know it's safe to release on any thread,
> tho.  But only if we have that information available 100% reliably - if, as
> you said, we called RetargetDeliveryTo with success _and_ the mListener has
> not changed since that or has not been added a new listener to the chain
> since that time.  That can IMO never be known for 100%.

sgtm, I was a bit worried that always release mListener on main thread is overkill.
Whiteboard: [necko-active]
@dragana, is there anything blocking the review? I'd like to resolve this issue before the merge day.
Flags: needinfo?(dd.mozilla)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> @dragana, is there anything blocking the review? I'd like to resolve this
> issue before the merge day.

I reviewed this on a train on Friday, and I submitted a review.... maybe I lost network connectivity and therefore the review has not been published... sorry... stupid moving trains....
Flags: needinfo?(dd.mozilla)
Comment on attachment 8888618 [details]
Bug 1382380 - ensure mListener/mListenerContext/mCompressListener is released on main thread.

https://reviewboard.mozilla.org/r/159628/#review166658
Attachment #8888618 - Flags: review?(dd.mozilla) → review+
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b01c620827b9
ensure mListener/mListenerContext/mCompressListener is released on main thread. r=dragana
Thanks for the promptly response, @dragana!
https://hg.mozilla.org/mozilla-central/rev/b01c620827b9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.