1155976 - Loading plugins in a non-e10s window hangs the browser on Mac

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

This shouldn't block e10s, the non-e10s window options will come out. Putting this on file though so we have a bug to point too.

typical stacks:

9f73d80e-9ed3-441c-b206-82b602150415

PLUGIN
-----------------------------------------------------------------------------------
PR_WaitCondVar
mozilla::ipc::MessageChannel::WaitForSyncNotify()
mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*)
mozilla::plugins::PPluginInstanceChild::SendShow(_NPRect const&, mozilla::plugins::SurfaceDescriptor const&, mozilla::plugins::SurfaceDescriptor*)
mozilla::plugins::PluginInstanceChild::ShowPluginFrame()
mozilla::plugins::PluginInstanceChild::InvalidateRectDelayed()
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&)
MessageLoop::DoWork()
base::MessagePumpCFRunLoopBase::RunWorkSource(void*)
base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*)
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*)
MessageLoop::Run()
XRE_InitChildProcess
content_process_main(int, char**)
start

BROWSER
-----------------------------------------------------------------------------------
google_breakpad::ExceptionHandler::WriteMinidump(bool)
google_breakpad::ExceptionHandler::WriteMinidump(std::string const&, bool, bool (*)(char const*, char const*, void*, bool), void*)
CrashReporter::CreatePairedMinidumps(unsigned int, unsigned int, nsIFile**)
mozilla::plugins::PluginModuleChromeParent::TerminateChildProcess(MessageLoop*)
mozilla::plugins::PluginModuleChromeParent::ShouldContinueFromReplyTimeout()
mozilla::plugins::PPluginModuleParent::OnReplyTimeout()
mozilla::ipc::MessageChannel::ShouldContinueFromTimeout()
mozilla::ipc::MessageChannel::Call(IPC::Message*, IPC::Message*)
mozilla::plugins::PPluginInstanceParent::CallNPP_HandleEvent(mozilla::plugins::NPRemoteEvent const&, short*)
mozilla::plugins::PluginInstanceParent::NPP_HandleEvent(void*)
mozilla::plugins::PluginModuleParent::NPP_HandleEvent(_NPP*, void*)
nsNPAPIPluginInstance::HandleEvent(void*, short*, NSPluginCallReentry)
nsPluginInstanceOwner::HandleEvent(nsIDOMEvent*)
mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*)
mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*)
mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)
mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, bool*)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, bool*)
nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*, bool)
nsContentUtils::DispatchEventOnlyToChrome(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*)
nsFocusManager::WindowRaised(nsIDOMWindow*)
nsWebShellWindow::WindowActivated()
-[WindowDelegate sendToplevelActivateEvents]
+[TopLevelWindowData activateInWindow:]
nsCocoaWindow::Show(bool)
nsXULWindow::Destroy()
nsWebShellWindow::Destroy()
nsWebShellWindow::RequestWindowClose(nsIWidget*)
_ZThn232_N16nsWebShellWindow18RequestWindowCloseEP9nsIWidget
-[WindowDelegate windowShouldClose:]
-[ToolbarWindow sendEvent:]
-[GeckoNSApplication sendEvent:]
nsAppShell::Run()
nsAppStartup::Run()
XREMain::XRE_mainRun()
XREMain::XRE_main(int, char**, nsXREAppData const*)
XRE_main
main
start

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I assume you have no STR here.

Are you simply tracking all cases where the browser "hangs", then kills the plugin because a timeout has expired?

The second stack from comment #0 makes it look like the "hang" takes place as a plugin is being unloaded (as the window which contains it is being destroyed).

Comment 2

[Jim Mathies [:jimm]]

(In reply to Steven Michaud from comment #1)
> I assume you have no STR here.
> 
> Are you simply tracking all cases where the browser "hangs", then kills the
> plugin because a timeout has expired?
> 
> The second stack from comment #0 makes it look like the "hang" takes place
> as a plugin is being unloaded (as the window which contains it is being
> destroyed).

Under e10s we should never see plugin code in browser stacks. Bill told me we have a known issue when people use the non-e10s window option and load a page with plugins. I filed this so we have something on it in bugzilla. FWIW I do not see a lot of these in nightly crash stats, I don't think it's serious.

We will not invest any time fixing this, the non-e10s window option is for testing only and will eventually come out.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Yes, I understood this bug is about non-e10s mode.

> Bill told me we have a known issue when people use the non-e10s
> window option and load a page with plugins.

But, like I said, the stacks in comment #0 seem to be about a hang
that takes place as a plugin is being unloaded.

I think the subject should be changed to something like "hangs in
non-e10s windows on plugin timeouts".  These hangs actually are a
fairly serious issue (in FF releases, at least).  And if e10s mode
fixes them, that's a pretty big deal.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Actually e10s-mode *doesn't* really fix these "hangs", so it isn't such a big deal.  I tested by attaching to a plugin process in lldb and not "continue"ing.  The browser chrome was usable, but no content was visible in any tab until I did "continue".

So no, this bug isn't really important :-(

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #3)
> Yes, I understood this bug is about non-e10s mode.

Just to clarify a bit more, this bug is about hangs that only happen if you're simultaneously using a plugin from an e10s window *and* from a non-e10s window. So if you have a Flash ad open in an e10s window and then visit Hulu from a non-e10s window.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Thanks, Bill.  Now this bug makes more sense :-)

Do you have reliable STR?  (I assume you don't.)

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Possibly related to bug 1155138?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: no_plugin_non-e10s.patch

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8596301 [details] [diff] [review]
no_plugin_non-e10s.patch

Review of attachment 8596301 [details] [diff] [review]:
-----------------------------------------------------------------

Have you pushed this to try? I'm wondering about tests, they do some weird stuff with plugins.

::: dom/base/nsObjectLoadingContent.cpp
@@ +3164,5 @@
>  
> +  if (XRE_GetProcessType() == GeckoProcessType_Default &&
> +             (Preferences::GetBool("browser.tabs.remote.autostart", false) ||
> +              Preferences::GetBool("browser.tabs.remote.autostart.1", false) ||
> +              Preferences::GetBool("browser.tabs.remote.autostart.2", false))) {

nit - your indentation is off here. Also typo in the comment, 'hang' vs. 'hage'.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d97c221555

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19d97c221555

Comment 12

[patwonder]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> Created attachment 8596301 [details] [diff] [review]
> no_plugin_non-e10s.patch

This patch breaks add-ons that bundles plugins with them, e.g. the IE Tab family (IE Tab, IE Tab V2, Fire IE, China Online Banking Assistant, etc.).

(In reply to Jim Mathies [:jimm] from comment #2)
> Under e10s we should never see plugin code in browser stacks.
This is not true. IE Tab add-ons typically uses a container page (that always loads in the UI process) to host a full page IE plugin to display web content. The patch disables the ability to load plugins in the UI process, thus breaking these add-ons badly.

(In reply to Jim Mathies [:jimm] from comment #2)
> We will not invest any time fixing this, the non-e10s window option is for
> testing only and will eventually come out.
Since the option is for testing only and will eventually come out, why not just WONTFIX this bug, instead of disabling the ability to load plugin in the UI process altogether?

Comment 13

[Jim Mathies [:jimm]]

> (In reply to Jim Mathies [:jimm] from comment #2)
> > Under e10s we should never see plugin code in browser stacks.
> This is not true. IE Tab add-ons typically uses a container page (that
> always loads in the UI process) to host a full page IE plugin to display web
> content. The patch disables the ability to load plugins in the UI process,
> thus breaking these add-ons badly.

This sound very insecure. Is this content hosted in some sort of a secure sandbox?

> (In reply to Jim Mathies [:jimm] from comment #2)
> > We will not invest any time fixing this, the non-e10s window option is for
> > testing only and will eventually come out.
> Since the option is for testing only and will eventually come out, why not
> just WONTFIX this bug, instead of disabling the ability to load plugin in
> the UI process altogether?

I think you are looking for bug 1158270.

Comment 14

[patwonder]

(In reply to Jim Mathies [:jimm] from comment #13)
> 
> > (In reply to Jim Mathies [:jimm] from comment #2)
> > > Under e10s we should never see plugin code in browser stacks.
> > This is not true. IE Tab add-ons typically uses a container page (that
> > always loads in the UI process) to host a full page IE plugin to display web
> > content. The patch disables the ability to load plugins in the UI process,
> > thus breaking these add-ons badly.
> 
> This sound very insecure. Is this content hosted in some sort of a secure
> sandbox?
> 

No. The web content is in plugin-container.exe, directly hosted by the container page. But these add-ons aren't about security anyway. They are about compatibility - some websites just won't work in Firefox no matter how hard you try.

> > (In reply to Jim Mathies [:jimm] from comment #2)
> > > We will not invest any time fixing this, the non-e10s window option is for
> > > testing only and will eventually come out.
> > Since the option is for testing only and will eventually come out, why not
> > just WONTFIX this bug, instead of disabling the ability to load plugin in
> > the UI process altogether?
> 
> I think you are looking for bug 1158270.

Bug 1158270 is about loading plugins in the UI process when e10s is (force-)disabled, while my concern is about loading plugins in the UI process even if e10s is enabled. The plugins that these add-ons bundles aren't used by websites, so they shouldn't hang the browser even without the patch in comment #8.

If you insist on not allowing plugins to run in the UI process, could we at least whitelist plugins that are bundled with add-ons, or simply have an entry in about:config to override the check?

Comment 15

[Jim Mathies [:jimm]]

(In reply to patwonder from comment #14)
> (In reply to Jim Mathies [:jimm] from comment #13)
> > 
> > > (In reply to Jim Mathies [:jimm] from comment #2)
> > > > Under e10s we should never see plugin code in browser stacks.
> > > This is not true. IE Tab add-ons typically uses a container page (that
> > > always loads in the UI process) to host a full page IE plugin to display web
> > > content. The patch disables the ability to load plugins in the UI process,
> > > thus breaking these add-ons badly.
> > 
> > This sound very insecure. Is this content hosted in some sort of a secure
> > sandbox?
> > 
> 
> No. The web content is in plugin-container.exe, directly hosted by the
> container page. But these add-ons aren't about security anyway. They are
> about compatibility - some websites just won't work in Firefox no matter how
> hard you try.

plugin-container will be sandboxed. How does your addon load content via IE into plugin_container? This seem like a rather odd configuration to me.

> > > (In reply to Jim Mathies [:jimm] from comment #2)
> > > > We will not invest any time fixing this, the non-e10s window option is for
> > > > testing only and will eventually come out.
> > > Since the option is for testing only and will eventually come out, why not
> > > just WONTFIX this bug, instead of disabling the ability to load plugin in
> > > the UI process altogether?
> > 
> > I think you are looking for bug 1158270.
> 
> Bug 1158270 is about loading plugins in the UI process when e10s is
> (force-)disabled, while my concern is about loading plugins in the UI
> process even if e10s is enabled. The plugins that these add-ons bundles
> aren't used by websites, so they shouldn't hang the browser even without the
> patch in comment #8.
>
> If you insist on not allowing plugins to run in the UI process.

Yes, absolutely. Especially any plugin that creates native UI which pretty much guarantees browser lock up at some point. This is one of the things e10s is supposed to help fix in Firefox. The last thing we want to do is turn this back on for the general public.

> could we at
> least whitelist plugins that are bundled with add-ons, or simply have an
> entry in about:config to override the check?

File a follow up I guess, discuss there with the security sandboxing team. I don't know what we can do here.

Comment 16

[patwonder]

(In reply to Jim Mathies [:jimm] from comment #15)
> plugin-container will be sandboxed. How does your addon load content via IE
> into plugin_container? This seem like a rather odd configuration to me.

The add-on uses a NPAPI plugin, which wraps a WebBrowser ActiveX control and serves as a bridge between add-on JS code and the IE WebBrowser instance. Since the NPAPI plugin is in plugin-container the IE ActiveX control is also in plugin-container.

> > could we at
> > least whitelist plugins that are bundled with add-ons, or simply have an
> > entry in about:config to override the check?
> 
> File a follow up I guess, discuss there with the security sandboxing team. I
> don't know what we can do here.

Filed bug 1158788.

Comment 17

[Jim Mathies [:jimm]]

(In reply to patwonder from comment #16)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > plugin-container will be sandboxed. How does your addon load content via IE
> > into plugin_container? This seem like a rather odd configuration to me.
> 
> The add-on uses a NPAPI plugin, which wraps a WebBrowser ActiveX control and
> serves as a bridge between add-on JS code and the IE WebBrowser instance.
> Since the NPAPI plugin is in plugin-container the IE ActiveX control is also
> in plugin-container.

Why do you need plugins to load in chrome then?

> 
> > > could we at
> > > least whitelist plugins that are bundled with add-ons, or simply have an
> > > entry in about:config to override the check?
> > 
> > File a follow up I guess, discuss there with the security sandboxing team. I
> > don't know what we can do here.
> 
> Filed bug 1158788.

Thanks!

Comment 18

[patwonder]

(In reply to Jim Mathies [:jimm] from comment #15)
> Especially any plugin that creates native UI which pretty
> much guarantees browser lock up at some point. This is one of the things
> e10s is supposed to help fix in Firefox.

Allow me to correct a few things. If you hope e10s could fix plugin hangs, you are probably wrong, at least on Windows.

When plugin creates native UI (almost all windowed plugins do this), it forms a parent/child relationship with the Firefox UI window, which is created by the chrome process. According to Raymond Chen's blog post (http://blogs.msdn.com/b/oldnewthing/archive/2013/04/12/10410454.aspx):

> Creating a cross-thread parent/child or owner/owned window relationship
> implicitly attaches the input queues of the threads which those windows
> belong to, and this attachment is transitive: If one of those queues is
> attached to a third queue, then all three queues are attached to each
> other. More generally, queues of all windows related by a chain of
> parent/child or owner/owned or shared-thread relationships are attached
> to each other.

When you have two threads' input queues attached, their message processing is effectively serialized (see http://blogs.msdn.com/b/oldnewthing/archive/2013/06/19/10426841.aspx for explanation). Thus when a windowed plugin hangs, it will hang Firefox UI window as well, whether it is loaded by e10s content process or not. 

(In reply to Jim Mathies [:jimm] from comment #17)
> Why do you need plugins to load in chrome then?
Let's first take a look at the add-on structure of Fire IE:

With plugins loaded in content processes:
| Process type: | chrome process | content process | plugin process |
| What's in it? | add-on JS code | container page  | web content    |

With plugins loaded in chrome processes:
| Process type: | chrome process                    | plugin process |
| What's in it? | add-on JS code and container page | web content    |

As for the Fire IE add-on, there are many reasons we don't want to load the plugin in e10s content processes:

1. Convenience. We have a lot of shared states in the chrome process. If plugins (and the container page) are loaded in content processes, we can't access them directly from the chrome process, and have to use the messaging API to talk to container pages. Similarly, when a plugin instance raises an event, it can't reach chrome process directly and also has to be passed through the messaging API.

We also have a global plugin instance to share plugin-specific states and data for all plugin instances. The global instance does not belong to any container pages, thus we can't load it in content processes.

2. Performance. With OOPP, accessing plugin data already have some IPC overhead. Accessing OOPP-enabled plugin loaded in content process from chrome scripts in the chrome process incurs 2 round-trip IPCs, which is unacceptable to us.

3. Necessity. Plugin-container already provides good crash protection for the web content. The only thing protected by content processes is the container page, which is unnecessary because it has no web content in it. Hang protection? I've explained earlier: e10s won't help fix windowed plugin hangs on Windows.