The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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)
(Reporter)

Updated

6 years ago
Component: IPC → Embedding: GTK Widget
QA Contact: ipc → gtk-widget
4) Whoever is loading a frame script over HTTP?

It doesn't look like the frame script executor is designed for that ...
bz, smaug, see comment 1.
(Reporter)

Updated

6 years ago
Component: Embedding: GTK Widget → IPC
QA Contact: gtk-widget → ipc
(Reporter)

Updated

6 years ago
Summary: Something needs to use nsContentUtils::AddScriptRunner → loadFrameScript should never use http
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.
(Reporter)

Comment 4

6 years ago
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.
Attachment #533368 - Flags: review?(mark.finkle)
Attachment #533368 - Flags: review?(Olli.Pettay)
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.
Attachment #533368 - Flags: review?(mark.finkle) → review+
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 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.
Attachment #533368 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 8

6 years ago
Indeed, I need to fix nsFrameScriptExecutor::LoadFrameScriptInternal to execute
scripts using script runner.
(Assignee)

Updated

6 years ago
Assignee: nobody → Olli.Pettay
That should allow syncloading http: at that point as well, too, right?
(Assignee)

Comment 10

6 years ago
Created attachment 533423 [details] [diff] [review]
patch

We could add perhaps some warning about non-local-resource uris.
Ben, could you test this?
Attachment #533423 - Flags: review?(ben)
(Assignee)

Comment 11

6 years ago
Er, not that patch
(Reporter)

Comment 12

6 years ago
Do you think we should add something that cancels the event in case we begin to shutdown?
(Assignee)

Updated

6 years ago
Attachment #533423 - Attachment is obsolete: true
Attachment #533423 - Flags: review?(ben)
(Assignee)

Comment 13

6 years ago
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.
Attachment #533426 - Flags: review?(ben)
(Reporter)

Comment 14

6 years ago
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?
(Assignee)

Comment 15

6 years ago
mTabChild is a strong pointer, so it can't become dangling pointer.
And LoadFrameScriptInternal returns early if TabChild has been disconnected.
(Assignee)

Updated

6 years ago
Attachment #533426 - Flags: review?(mark.finkle)
Attachment #533426 - Flags: review?(bzbarsky)
Attachment #533426 - Flags: review?(ben)
Comment on attachment 533426 [details] [diff] [review]
patch

r=me
Attachment #533426 - Flags: review?(bzbarsky) → review+
Comment on attachment 533426 [details] [diff] [review]
patch

We'll keep watch for any fallout
Attachment #533426 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

6 years ago
Summary: loadFrameScript should never use http → loadFrameScript should never use http and should use a script runner when needed
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/d691bdf1ff43
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 658941
You need to log in before you can comment on or make changes to this bug.