657997 - loadFrameScript should never use http and should use a script runner when needed

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Stover (:stechz)]

On Linux GTK, it is possible to receive a raise or lower event while a script is being run. This happens because sync stream listener uses process next native event, which in this case causes a window focus to occur.

Who is at blame here?
1) Sync stream listener for using process next native event?
2) The focus manager for not expecting content scripts to be running?
3) The GTK widget for not expecting content scripts to be running?

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /home/ben/projects/mc/content/events/src/nsEventDispatcher.cpp, line 534
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsPIDOMEventTarget>*) (/home/ben/projects/mc/content/events/src/nsEventDispatcher.cpp:535)
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (/home/ben/projects/mc/content/events/src/nsEventDispatcher.cpp:711)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, int*) (/home/ben/projects/mc/dom/base/nsGlobalWindow.cpp:7115)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, int*) (/home/ben/projects/mc/dom/base/nsGlobalWindow.cpp:7098)
nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, int, int, int*) (/home/ben/projects/mc/content/base/src/nsContentUtils.cpp:3387)
nsFocusManager::WindowLowered(nsIDOMWindow*) (/home/ben/projects/mc/dom/base/nsFocusManager.cpp:779)
nsWebShellWindow::HandleEvent(nsGUIEvent*) (/home/ben/projects/mc/xpfe/appshell/src/nsWebShellWindow.cpp:472)
nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) (/home/ben/projects/mc/widget/src/gtk2/nsWindow.cpp:591)
nsWindow::DispatchDeactivateEvent() (/home/ben/projects/mc/widget/src/gtk2/nsWindow.cpp:573)
nsWindow::OnContainerFocusOutEvent(_GtkWidget*, _GdkEventFocus*) (/home/ben/projects/mc/widget/src/gtk2/nsWindow.cpp:2983)
focus_out_event_cb (/home/ben/projects/mc/widget/src/gtk2/nsWindow.cpp:5731)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
g_closure_invoke+0x000001B2  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit_valist+0x000005D3  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit+0x00000026  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_closure_invoke+0x000001B2  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit_valist+0x000005D3  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit+0x00000026  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
gtk_main_do_event+0x0000043C  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgdk-x11-2.0.so.0)
g_main_context_dispatch+0x000001D5  (/lib/libglib-2.0.so.0)
UNKNOWN  (/lib/libglib-2.0.so.0)
g_main_context_iteration+0x00000068  (/lib/libglib-2.0.so.0)
nsAppShell::ProcessNextNativeEvent(int) (/home/ben/projects/mc/widget/src/gtk2/nsAppShell.cpp:145)
nsBaseAppShell::DoProcessNextNativeEvent(int) (/home/ben/projects/mc/widget/src/xpwidgets/nsBaseAppShell.cpp:171)
nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, int, unsigned int) (/home/ben/projects/mc/widget/src/xpwidgets/nsBaseAppShell.cpp:324)
mozilla::dom::ContentParent::OnProcessNextEvent(nsIThreadInternal*, int, unsigned int) (/home/ben/projects/mc/dom/ipc/ContentParent.cpp:968)
nsThread::ProcessNextEvent(int, int*) (/home/ben/projects/mc/xpcom/threads/nsThread.cpp:584)
NS_ProcessNextEvent_P(nsIThread*, int) (/home/ben/projects/mc/obj-mobile/xpcom/build/nsThreadUtils.cpp:250)
nsSyncStreamListener::WaitForData() (/home/ben/projects/mc/netwerk/base/src/nsSyncStreamListener.cpp:58)
nsSyncStreamListener::Available(unsigned int*) (/home/ben/projects/mc/netwerk/base/src/nsSyncStreamListener.cpp:160)
NS_ImplementChannelOpen(nsIChannel*, nsIInputStream**) (/home/ben/projects/mc/obj-mobile/netwerk/protocol/http/../../../dist/include/nsNetUtil.h:670)
mozilla::net::HttpBaseChannel::Open(nsIInputStream**) (/home/ben/projects/mc/netwerk/protocol/http/HttpBaseChannel.cpp:396)
nsFrameScriptExecutor::LoadFrameScriptInternal(nsAString_internal const&) (/home/ben/projects/mc/content/base/src/nsFrameMessageManager.cpp:689)
nsInProcessTabChildGlobal::LoadFrameScript(nsAString_internal const&) (/home/ben/projects/mc/content/base/src/nsInProcessTabChildGlobal.cpp:342)
LoadScript(void*, nsAString_internal const&) (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:1909)
nsFrameMessageManager::LoadFrameScript(nsAString_internal const&, int) (/home/ben/projects/mc/content/base/src/nsFrameMessageManager.cpp:163)
nsFrameMessageManager::AddChildManager(nsFrameMessageManager*, int) (/home/ben/projects/mc/content/base/src/nsFrameMessageManager.cpp:488)
nsFrameMessageManager::SetCallbackData(void*, int) (/home/ben/projects/mc/content/base/src/nsFrameMessageManager.cpp:503)
nsFrameLoader::EnsureMessageManager() (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:2070)
nsFrameLoader::MaybeCreateDocShell() (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:1512)
nsFrameLoader::CheckURILoad(nsIURI*) (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:524)
nsFrameLoader::LoadURI(nsIURI*) (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:412)
nsFrameLoader::LoadFrame() (/home/ben/projects/mc/content/base/src/nsFrameLoader.cpp:377)
nsXULElement::LoadSrc() (/home/ben/projects/mc/content/xul/content/src/nsXULElement.cpp:2009)
nsXULElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, int) (/home/ben/projects/mc/content/xul/content/src/nsXULElement.cpp:914)
nsGenericElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, int) (/home/ben/projects/mc/content/base/src/nsGenericElement.cpp:3022)
nsStyledElementNotElementCSSInlineStyle::BindToTree(nsIDocument*, nsIContent*, nsIContent*, int) (/home/ben/projects/mc/content/base/src/nsStyledElement.cpp:229)
nsXULElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, int) (/home/ben/projects/mc/content/xul/content/src/nsXULElement.cpp:904)
nsINode::doInsertChildAt(nsIContent*, unsigned int, int, nsAttrAndChildArray&) (/home/ben/projects/mc/content/base/src/nsGenericElement.cpp:3629)
nsGenericElement::InsertChildAt(nsIContent*, unsigned int, int) (/home/ben/projects/mc/content/base/src/nsGenericElement.cpp:3552)
nsINode::ReplaceOrInsertBefore(int, nsINode*, nsINode*) (/home/ben/projects/mc/content/base/src/nsGenericElement.cpp:4262)
nsINode::ReplaceOrInsertBefore(int, nsINode*, nsINode*, unsigned int*) (/home/ben/projects/mc/obj-mobile/js/src/xpconnect/src/../../../../dist/include/nsINode.h:1267)
nsINode::InsertBefore(nsINode*, nsINode*, unsigned int*) (/home/ben/projects/mc/obj-mobile/js/src/xpconnect/src/../../../../dist/include/nsINode.h:445)
nsIDOMNode_InsertBefore (/home/ben/projects/mc/obj-mobile/js/src/xpconnect/src/dom_quickstubs.cpp:6370)
js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, js::Value*) (/home/ben/projects/mc/js/src/jscntxtinlines.h:277)
.L4869 (/home/ben/projects/mc/js/src/jsinterp.cpp:4673)
js::RunScript(JSContext*, JSScript*, js::StackFrame*) (/home/ben/projects/mc/js/src/jsinterp.cpp:604)
js::Invoke(JSContext*, js::CallArgs const&, js::ConstructOption) (/home/ben/projects/mc/js/src/jsinterp.cpp:684)
js_fun_call(JSContext*, unsigned int, js::Value*) (/home/ben/projects/mc/js/src/jsfun.cpp:2149)
js_fun_apply(JSContext*, unsigned int, js::Value*) (/home/ben/projects/mc/js/src/jsfun.cpp:2167)
js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, js::Value*) (/home/ben/projects/mc/js/src/jscntxtinlines.h:277)
.L4869 (/home/ben/projects/mc/js/src/jsinterp.cpp:4673)
js::RunScript(JSContext*, JSScript*, js::StackFrame*) (/home/ben/projects/mc/js/src/jsinterp.cpp:604)
js::Invoke(JSContext*, js::CallArgs const&, js::ConstructOption) (/home/ben/projects/mc/js/src/jsinterp.cpp:684)
js::ExternalInvoke(JSContext*, js::Value const&, js::Value const&, unsigned int, js::Value*, js::Value*) (/home/ben/projects/mc/js/src/jsinterp.cpp:806)
JS_CallFunctionValue (/home/ben/projects/mc/js/src/jsapi.cpp:5081)
nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) (/home/ben/projects/mc/dom/base/nsJSEnvironment.cpp:1903)
nsGlobalWindow::RunTimeout(nsTimeout*) (/home/ben/projects/mc/dom/base/nsGlobalWindow.cpp:9128)
nsGlobalWindow::TimerCallback(nsITimer*, void*) (/home/ben/projects/mc/dom/base/nsGlobalWindow.cpp:9476)
nsTimerImpl::Fire() (/home/ben/projects/mc/xpcom/threads/nsTimerImpl.cpp:425)
nsTimerEvent::Run() (/home/ben/projects/mc/xpcom/threads/nsTimerImpl.cpp:522)
nsThread::ProcessNextEvent(int, int*) (/home/ben/projects/mc/xpcom/threads/nsThread.cpp:618)
NS_ProcessNextEvent_P(nsIThread*, int) (/home/ben/projects/mc/obj-mobile/xpcom/build/nsThreadUtils.cpp:250)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ben/projects/mc/ipc/glue/MessagePump.cpp:107)
MessageLoop::RunInternal() (/home/ben/projects/mc/ipc/chromium/src/base/message_loop.cc:219)
MessageLoop::RunHandler() (/home/ben/projects/mc/ipc/chromium/src/base/message_loop.cc:203)
MessageLoop::Run() (/home/ben/projects/mc/ipc/chromium/src/base/message_loop.cc:176)
nsBaseAppShell::Run() (/home/ben/projects/mc/widget/src/xpwidgets/nsBaseAppShell.cpp:191)
nsAppStartup::Run() (/home/ben/projects/mc/toolkit/components/startup/nsAppStartup.cpp:224)
XRE_main (/home/ben/projects/mc/toolkit/xre/nsAppRunner.cpp:3698)
main (/home/ben/projects/mc/mobile/app/nsBrowserApp.cpp:155)
__libc_start_main (/build/buildd/eglibc-2.11.1/csu/libc-start.c:258)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

4) Whoever is loading a frame script over HTTP?

It doesn't look like the frame script executor is designed for that ...

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

bz, smaug, see comment 1.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

As I alluded to on IRC, I think we should have a whitelist of protocols here and abort if we get one we don't know about.

Comment 4

[Benjamin Stover (:stechz)]

Attachment: loadFrameScript should never use http

As Kyle helpfully pointed out, loadFrameScript expects to load files
synchronously and never expects them to be remote. http causes our event loop
to spin, which makes lots of bad things happen.

Mark, there was a comment in remote_head.js as to why we were using http, but
A) that test is disabled and B) that code might have fixed a test, but it did
this by making things happen that should never happen.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 533368 [details] [diff] [review]
loadFrameScript should never use http

fine by me. we can fix up any test fallout if it happens.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, the real issue here is an attempt to do synchronous loading at a spot when it's not safe to spin the event loop without making sure that the load can really happen sync.

But there's a higher-level issue here: this stack shows that nsFrameScriptExecutor::LoadFrameScriptInternal is trying to RUN scripts during BindToTree.  DO NOT DO THAT.  Ever.  Just don't.

What is this code trying to do?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533368 [details] [diff] [review]
loadFrameScript should never use http

This is wrong for at least the following reasons:

1)  Why not file:// ?
2)  Use protocol flags, not scheme checks (see #1).
3)  This does not solve the fundamental problem of running script at times when
    script can't run safely.  That's the problem that needs solving here.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Indeed, I need to fix nsFrameScriptExecutor::LoadFrameScriptInternal to execute
scripts using script runner.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That should allow syncloading http: at that point as well, too, right?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

We could add perhaps some warning about non-local-resource uris.
Ben, could you test this?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Er, not that patch

Comment 12

[Benjamin Stover (:stechz)]

Do you think we should add something that cancels the event in case we begin to shutdown?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This one. And no, there shouldn't be any reason to cancel the runnable.
Ben, please test this.

Comment 14

[Benjamin Stover (:stechz)]

Comment on attachment 533426 [details] [diff] [review]
patch

This fixes the assertion for me.

Not sure if you are asking for a proper review request from me since I don't know this code well, but here are my thoughts anyway.

What if disconnect or shutdown is called? For instance:
1) script calls loadFrameScript on a window
2) window is immediately closed

I know it's possible in certain cases for window.close() to close the window immediately. mTabChild could very potentially be a dangling pointer by the time the event is run, couldn't it?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

mTabChild is a strong pointer, so it can't become dangling pointer.
And LoadFrameScriptInternal returns early if TabChild has been disconnected.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533426 [details] [diff] [review]
patch

r=me

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 533426 [details] [diff] [review]
patch

We'll keep watch for any fallout

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/d691bdf1ff43