Closed Bug 548451 Opened 14 years ago Closed 14 years ago

accept 3rd-party cookies + prompt for all cookies + Flagfox 4.0.0 = Firefox crash [@ nsXPConnect::Peek(JSContext**) ] [@ JS_RestoreFrameChain ] [@ JS_ResumeRequest ]

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Version:
1.9.1 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: davemgarrett, Unassigned)

References

Details

If you set Firefox to the (annoying) mode of prompting for all cookies and also have 3rd-party cookies enabled, then install Flagfox 4.0.0, Firefox will usually crash on startup. Flagfox is pure JavaScript; it has no compiled modules.
https://addons.mozilla.org/en-US/firefox/addons/versions/5791#version-4.0.0

It's triggered on preloading the favicons for the context menu. Disabling either 3rd-party cookies or prompting for all cookies will prevent the crash. Disabling favicons in the Flagfox options also avoids the crash.

This is usually the stack I get under Linux:
bp-c173dbb7-c688-4de0-9582-167502100224
0  	libxul.so  	nsXPConnect::Peek  	 js/src/xpconnect/src/nsXPConnect.cpp:2557
1 	libxul.so 	nsCxPusher::DoPush 	content/base/src/nsContentUtils.cpp:2712
2 	libxul.so 	nsCxPusher::Push 	content/base/src/nsContentUtils.cpp:2827
3 	libxul.so 	nsCxPusher::Push 	content/base/src/nsContentUtils.cpp:2774
4 	libxul.so 	nsXBLProtoImplAnonymousMethod::Execute 	content/xbl/src/nsXBLProtoImplMethod.cpp:322
5 	libxul.so 	nsXBLPrototypeBinding::BindingAttached 	content/xbl/src/nsXBLPrototypeBinding.cpp:504
6 	libxul.so 	nsXBLBinding::ExecuteAttachedHandler 	content/xbl/src/nsXBLBinding.cpp:977
7 	libxul.so 	nsRunnableMethod<nsXBLBinding, void>::Run 	nsThreadUtils.h:282
8 	libxul.so 	nsContentUtils::RemoveScriptBlocker 	content/base/src/nsContentUtils.cpp:4484
9 	libxul.so 	DocumentViewerImpl::InitInternal 	nsContentUtils.h:1659
10 	libxul.so 	DocumentViewerImpl::Init 	layout/base/nsDocumentViewer.cpp:698
11 	libxul.so 	nsDocShell::SetupNewViewer 	docshell/base/nsDocShell.cpp:7298
12 	libxul.so 	nsDocShell::Embed 	docshell/base/nsDocShell.cpp:5463
13 	libxul.so 	nsDocShell::CreateAboutBlankContentViewer 	docshell/base/nsDocShell.cpp:6171
14 	libxul.so 	nsDocShell::EnsureContentViewer 	docshell/base/nsDocShell.cpp:6070
15 	libxul.so 	nsDocShell::GetInterface 	docshell/base/nsDocShell.cpp:857
16 	libxul.so 	nsGetInterface::operator const 	nsIInterfaceRequestorUtils.cpp:52
17 	libxul.so 	nsCOMPtr_base::assign_from_helper 	nsCOMPtr.cpp:150
18 	libxul.so 	nsGlobalWindow::GetDocument 	nsCOMPtr.h:621
19 	libxul.so 	nsDataDocumentContentPolicy::ShouldLoad 	content/base/src/nsDataDocumentContentPolicy.cpp:71
20 	libxul.so 	nsContentPolicy::CheckPolicy 	content/base/src/nsContentPolicy.cpp:157
21 	libxul.so 	nsContentPolicy::ShouldLoad 	content/base/src/nsContentPolicy.cpp:218
22 	libxul.so 	NS_CheckContentLoadPolicy 	nsContentPolicyUtils.h:223
23 	libxul.so 	nsDocShell::InternalLoad 	docshell/base/nsDocShell.cpp:7575
24 	libxul.so 	nsDocShell::LoadURI 	docshell/base/nsDocShell.cpp:1354
25 	libxul.so 	nsWindowWatcher::OpenWindowJSInternal 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:942
26 	libxul.so 	nsWindowWatcher::OpenWindow 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:424
27 	libxul.so 	nsCookiePromptService::CookieDialog 	extensions/cookie/nsCookiePromptService.cpp:114
28 	libxul.so 	nsCookiePermission::CanSetCookie 	extensions/cookie/nsCookiePermission.cpp:355
29 	libxul.so 	nsCookieService::SetCookieInternal 	netwerk/cookie/src/nsCookieService.cpp:1482
30 	libxul.so 	nsCookieService::SetCookieStringInternal 	netwerk/cookie/src/nsCookieService.cpp:802
31 	libxul.so 	nsCookieService::SetCookieStringFromHttp 	netwerk/cookie/src/nsCookieService.cpp:757
32 	libxul.so 	nsHttpChannel::SetCookie 	netwerk/protocol/http/src/nsHttpChannel.cpp:5037
33 	libxul.so 	nsHttpChannel::ProcessResponse 	netwerk/protocol/http/src/nsHttpChannel.cpp:990
34 	libxul.so 	nsHttpChannel::OnStartRequest 	netwerk/protocol/http/src/nsHttpChannel.cpp:5157
35 	libxul.so 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:439
36 	libxul.so 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:395
37 	libxul.so 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:112
38 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
39 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
40 	libxul.so 	nsSyncStreamListener::WaitForData 	netwerk/base/src/nsSyncStreamListener.cpp:58
41 	libxul.so 	nsSyncStreamListener::Available 	netwerk/base/src/nsSyncStreamListener.cpp:160
42 	libxul.so 	NS_ImplementChannelOpen 	nsNetUtil.h:619
43 	libxul.so 	nsHttpChannel::Open 	netwerk/protocol/http/src/nsHttpChannel.cpp:4403
44 	libxul.so 	NS_GetXPTCallStub_P 	
45 	libxul.so 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2721
46 	libxul.so 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
47 	libmozjs.so 	js_Invoke 	js/src/jsinterp.cpp:1360
48 	libmozjs.so 	js_Interpret 	js/src/jsops.cpp:2240
49 	libmozjs.so 	js_Invoke 	js/src/jsinterp.cpp:1368
50 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696
51 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:570
52 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95
53 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
54 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
55 	libxul.so 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:254
56 	libnspr4.so 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c:228
57 	libpthread-2.7.so 	libpthread-2.7.so@0x54fa

Also got this one once:
bp-48bee7b3-bda6-4d46-84cd-df8562100224
0  	libmozjs.so  	JS_RestoreFrameChain  	 js/src/jsapi.cpp:5220
1 	libxul.so 	XPCJSContextStack::Pop 	js/src/xpconnect/src/xpcthreadcontext.cpp:107
2 	libxul.so 	nsXPConnect::Pop 	js/src/xpconnect/src/nsXPConnect.cpp:2573
3 	libxul.so 	nsXPConnect::AfterProcessNextEvent 	js/src/xpconnect/src/nsXPConnect.cpp:2316
4 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:540
5 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
6 	libxul.so 	nsXULWindow::ShowModal 	xpfe/appshell/src/nsXULWindow.cpp:416
7 	libxul.so 	nsContentTreeOwner::ShowAsModal 	xpfe/appshell/src/nsContentTreeOwner.cpp:528
8 	libxul.so 	nsWindowWatcher::OpenWindowJSInternal 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:1003
9 	libxul.so 	nsWindowWatcher::OpenWindow 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:424
... (same as above afterwards) ...

Under Windows most tests ended in a hang. Got this one crash report:
bp-b32cdb5b-cbca-4b05-823f-005e92100224
0  	js3250.dll  	JS_ResumeRequest  	 js/src/jsapi.cpp:1011
1 	xul.dll 	nsXPConnect::Pop 	js/src/xpconnect/src/nsXPConnect.cpp:2502
2 	xul.dll 	nsCxPusher::Pop 	content/base/src/nsContentUtils.cpp:2804
3 	xul.dll 	nsCxPusher::~nsCxPusher 	content/base/src/nsContentUtils.cpp:2669
4 	xul.dll 	nsXBLBinding::ExecuteAttachedHandler 	content/xbl/src/nsXBLBinding.cpp:978
5 	xul.dll 	nsBindingManager::ProcessAttachedQueue 	content/xbl/src/nsBindingManager.cpp:996
6 	xul.dll 	PresShell::InitialReflow 	layout/base/nsPresShell.cpp:2697
7 	xul.dll 	nsXULDocument::StartLayout 	content/xul/document/src/nsXULDocument.cpp:2064
8 	xul.dll 	nsXULDocument::DoneWalking 	content/xul/document/src/nsXULDocument.cpp:3208
9 	xul.dll 	nsXULDocument::ResumeWalk 	content/xul/document/src/nsXULDocument.cpp:3157
10 	mozcrt19.dll 	free 	obj-firefox/memory/jemalloc/src/jemalloc.c:6404
11 	xul.dll 	nsCOMPtr_base::~nsCOMPtr_base 	obj-firefox/xpcom/build/nsCOMPtr.cpp:81
12 	xul.dll 	nsXULDocument::OnStreamComplete 	content/xul/document/src/nsXULDocument.cpp:3611
13 	xul.dll 	nsStreamLoader::OnStopRequest 	netwerk/base/src/nsStreamLoader.cpp:108
14 	xul.dll 	nsJARChannel::OnStopRequest 	modules/libjar/nsJARChannel.cpp:877
15 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
16 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
17 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
18 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:521
19 	xul.dll 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:227
20 	xul.dll 	nsXULWindow::ShowModal 	xpfe/appshell/src/nsXULWindow.cpp:415
21 	xul.dll 	nsContentTreeOwner::ShowAsModal 	xpfe/appshell/src/nsContentTreeOwner.cpp:528
22 	xul.dll 	xul.dll@0x3613f1 	
23 	xul.dll 	nsWindowWatcher::OpenWindow 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:422
24 	xul.dll 	nsCookiePromptService::CookieDialog 	extensions/cookie/nsCookiePromptService.cpp:114
25 	xul.dll 	xul.dll@0x30d4f8 	
26 		@0x360f217 	
27 	nspr4.dll 	nspr4.dll@0x1758f 	
28 	xul.dll 	nsCookieService::SetCookieInternal 	netwerk/cookie/src/nsCookieService.cpp:1401
29 	xul.dll 	nsCookieService::SetCookieInternal 	netwerk/cookie/src/nsCookieService.cpp:1416

I'm in the process of releasing a Flagfox 4.0.1 update that will disable favicon preloading with this set of cookie options on. Even if it didn't crash it'd be annoying with all the popups on startup. Not quite sure why it's even monkeying around with cookies at all, as I'm just using a favicon image.
> If you set Firefox to the (annoying) mode of prompting for all cookies

This mode isn't supported, and has no UI to set it, last I checked, precisely because it can lead to crash issues.
(In reply to comment #1)
> > If you set Firefox to the (annoying) mode of prompting for all cookies
> 
> This mode isn't supported, and has no UI to set it, last I checked, precisely
> because it can lead to crash issues.

Yes there is. Under Firefox 3.5/3.6 options/preferences -> Privacy tab -> select "use custom settings for history" -> check "accept cookies from sites" -> check "accept third-party cookies" -> set "keep until" to "ask me every time".

You'll start getting popup dialogs like crazy when you browse. This is in there in some form in Firefox 3.0, 3.5, 3.6, and Trunk.
> set "keep until" to "ask me every time".

Oh, that's very unfortunate.  I was pretty serious about that mode not being supported in the core code...
Yeah. We're removing it :)
Depends on: 546746
Looks like SeaMonkey 2.0 has this in its options too. Go to preferences -> Privacy & Security -> Cookies -> select "allow all cookies" and select "ask for each cookie". Checking "except for session cookies" seems to avoid the problem, but leaving it unchecked will trigger the same hang or crash.
(In reply to comment #4)
> Yeah. We're removing it :)

I don't suppose a fix to this crash is possible in the meantime?
Perhaps. CC'ing mrbkap, though this probably doesn't belong in XPConnect; bz may also know.

We can't really disable the feature on a stable branch, so we're stuck with a crash fix. :(
>> set "keep until" to "ask me every time".

> Oh, that's very unfortunate.  I was pretty serious about that mode not being
> supported in the core code...

I think this was added in Bug 233339 by mconnor.
This seems to be enough by itself to get a hang:

var ioService = Components.classes["@mozilla.org/network/io-service;1"]
                 .getService(Components.interfaces.nsIIOService);
var threadManager = Components.classes["@mozilla.org/thread-manager;1"]
                     .getService(Components.interfaces.nsIThreadManager);
var uri = ioService.newURI("http://www.mywot.com/favicon.ico", null, null);
var channel = ioService.newChannelFromURI(uri);
var thread = threadManager.newThread(0);
thread.dispatch( { run : function() { channel.open(); } },
                 thread.DISPATCH_NORMAL );

Substitute in google.com or disable 3rd-party cookies or disable prompting for cookies and it will work fine. It's that for some reason with 3rd-party cookies turned on it feels the need to set a cookie for the mywot.com favicon. I think the popup of the cookie dialog being loaded from the thread is what's triggering things.
Uh... where is the code in comment 9 running, exactly?  Is that similar to code that Flagfox uses?
Yes, similar but reduced.
Is that running in a JS component, or in a Window?
Comes from a window. Main code is in a JavaScript module imported into an overlay using Components.utils.import(). comment 9 will break things if it's done from there or in the overlay itself.

I do close the opened channels in Flagfox, by the way.
In a window that code _will_ hang and likely crash.  Windows are not threadsafe, and you're passing the window over to your other thread as part of the scope of your runnable.  That code is only safe in a JS component, but....

As far as that goes, nsIChannel is also not safe to use from off the main thread.  Using it from other threads will lead to various forms of misbehavior, hangs, and maybe crashes.

In particular, your code in this case is putting up UI from off the main thread, which is completely unsupported and _will_ crash (as you discovered).
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
MDC only noted to not use the DOM or UI. It didn't say the thing was this anywhere near this dangerous. I was under the impression that extensions shouldn't be capable of causing crashes from JavaScript like this.
You're using UI, though.  Not directly, but you're doing it.  Those cookie dialogs are coming up on your worker thread.  And the Window is the DOM, and the function you pass has the Window as its scope object.  So for example the lookup of the "channel" property involves the Window.  So you are in fact using the DOM.

> extensions shouldn't be capable of causing crashes from JavaScript like this

In theory, yes. In practice, the only way to prevent that is to disallow use of threads from extensions, period or to make all code through the codebase that an extension can touch (so the DOM and UI, in particular) fully threadsafe.
What you're describing to me sounds like virtually everything one could do in a thread like this would have the potential to hang or crash.
Threads are fine for i/o (but not network access) and for computation of various sorts.  They're not fine, as currently implemented, for touching the network or the UI or the DOM in any way.
MDC seems to be woefully missing an explanation of what is safe vs. dangerous here.
https://developer.mozilla.org/en/The_Thread_Manager
I see "You must not access the DOM or user interface from background threads. Doing so will likely crash." which doesn't adequately explain the scope of the apparent risk here. If I knew how easy it was to explode I wouldn't have used it.

The part that's really annoying me is that I had 100-200 beta testers with this thing for 3 weeks and nobody reported any startup crashes.

Some users are stating in reviews that 4.0.1 didn't fix their problem so I've posted a version 4.0.2 with this disabled entirely and have sandboxed the 4.0.0 version to prevent updates to it by anyone who hasn't done so yet.

Thanks for your explanations.
Dave, that page is a wiki.  You should be able to edit it to make it clearer as needed; I'll try to make time to do that too.
I know, I've edited MDC pages before. After you add any more info you wish to add there, I'll go through some other pages referencing this and amend them with more informative warnings. Thanks.
OK.  I just went and looked at the mdc page and it says:

  Note: The DOM is not thread safe. You must not access the DOM or user interface
  from background threads. Doing so will likely crash. This also includes not
  being on a different thread where the DOM is reachable in your scope, such as
  scripts included in documents.

That last sentence pretty much covers the "don't run in a Window" case, no?  That's exactly what it's talking about...

I added a note about channels.
In Flagfox I was doing the crashing code in a JavaScript module, not called directly in a window. It did not have access to the scope of the window where run, at least to my knowledge. (no 'window' object available in JSM) If I'm misunderstanding this then it might be a good idea to better explain the issues with scope in the warning in a more detailed way to avoid accidents.

The wording here is a bit clunky: "includes not being on a different thread where the DOM is reachable in your scope". To me I read that as including the instance where DOM is reachable in the calling scope while also being on the same thread. It was in the same thread but did not use DOM.

The big miss-communication came from the fact that this is a mere note and does not warn of the potential unpredictability here. I used it, tested it, and had well over a hundred more test it, but did not have any problems because something unexpected happened that nobody else did: the cookie prompting.

The one thing that is acceptable to crash from JavaScript needs to have a big and unambiguous warning, red box and all.

I also think that your mention of not allowing usage of this by extensions is actually a good idea. I see there's also web workers in Gecko 1.9.1, so it would make sense to replace usage with that instead if it's safer and is capable of doing the same things.
> not called directly in a window

Hmm.  I'm not sure how the JS module setup works, actually.

And yeah, there are plans to make it easier to do web-worker-like shared-nothing things in chrome, which will make these pitfalls go away.  See the recent "Proposed changes to JS_THREADSAFE rules" thread in mozilla.dev.tech.js-engine.
https://developer.mozilla.org/en/nsIChannel
Forgot about this note. For nsIChannel.open() it says:
"Moreover, since this method may block the calling thread, it should not be called on a thread that processes UI events."

You added "Currently nsIChannel APIs can only be used from the main thread." to the Thread Manager article. First one seems to be bad info if this is the case. I don't remember exactly, but that might've been something I read that pointed me in the direction of multithreading things in the first place. (using asyncOpen was a more complex route)
And that comment is pulled from current source:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannel.idl
Roughly people are supposed to use nsIChannel.asyncOpen (from the main thread). And should never use nsIChannel.open, but it's there because well, because it's there for trouble makers.
> https://developer.mozilla.org/en/nsIChannel

I've updated the documentation.

> First one seems to be bad info if this is the case.

Technically, the XPCOM main thread need not be the thread the UI runs on in non-xulrunner embeddings...  I've clarified the note here and in the idl to make things clearer.
You need to log in before you can comment on or make changes to this bug.