Last Comment Bug 657997 - loadFrameScript should never use http and should use a script runner when needed
: loadFrameScript should never use http and should use a script runner when needed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on: 658941
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-18 11:29 PDT by Benjamin Stover (:stechz)
Modified: 2011-05-22 22:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
loadFrameScript should never use http (2.12 KB, patch)
2011-05-18 12:17 PDT, Benjamin Stover (:stechz)
bzbarsky: review-
mark.finkle: review+
Details | Diff | Review
patch (2.48 KB, patch)
2011-05-18 14:23 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
patch (2.45 KB, patch)
2011-05-18 14:33 PDT, Olli Pettay [:smaug]
mark.finkle: review+
bzbarsky: review+
Details | Diff | Review

Description Benjamin Stover (:stechz) 2011-05-18 11:29:48 PDT
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 [:khuey] (khuey@mozilla.com) 2011-05-18 11:36:16 PDT
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 [:khuey] (khuey@mozilla.com) 2011-05-18 11:36:37 PDT
bz, smaug, see comment 1.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-18 12:15:02 PDT
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) 2011-05-18 12:17:04 PDT
Created attachment 533368 [details] [diff] [review]
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?) 2011-05-18 12:24:57 PDT
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 [:bz] 2011-05-18 12:27:41 PDT
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 [:bz] 2011-05-18 12:29:05 PDT
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] 2011-05-18 12:31:32 PDT
Indeed, I need to fix nsFrameScriptExecutor::LoadFrameScriptInternal to execute
scripts using script runner.
Comment 9 Boris Zbarsky [:bz] 2011-05-18 12:33:24 PDT
That should allow syncloading http: at that point as well, too, right?
Comment 10 Olli Pettay [:smaug] 2011-05-18 14:23:12 PDT
Created attachment 533423 [details] [diff] [review]
patch

We could add perhaps some warning about non-local-resource uris.
Ben, could you test this?
Comment 11 Olli Pettay [:smaug] 2011-05-18 14:24:50 PDT
Er, not that patch
Comment 12 Benjamin Stover (:stechz) 2011-05-18 14:25:33 PDT
Do you think we should add something that cancels the event in case we begin to shutdown?
Comment 13 Olli Pettay [:smaug] 2011-05-18 14:33:29 PDT
Created attachment 533426 [details] [diff] [review]
patch

This one. And no, there shouldn't be any reason to cancel the runnable.
Ben, please test this.
Comment 14 Benjamin Stover (:stechz) 2011-05-18 16:30:27 PDT
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] 2011-05-19 02:24:55 PDT
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 [:bz] 2011-05-19 06:51:38 PDT
Comment on attachment 533426 [details] [diff] [review]
patch

r=me
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 07:04:30 PDT
Comment on attachment 533426 [details] [diff] [review]
patch

We'll keep watch for any fallout
Comment 18 Olli Pettay [:smaug] 2011-05-19 07:51:47 PDT
http://hg.mozilla.org/mozilla-central/rev/d691bdf1ff43

Note You need to log in before you can comment on or make changes to this bug.