544945 - [OOPP] Detect when Gtk plugins enter a nested event loop

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

When this happens, we'll want to clear the hung-plugin timeout and spin the event loop in the browser process.

Karl proposed enqueuing a high-priority glib event, before calling into plugin code, to detect nested loops.  Karl, could this work for the case where a plugin's own glib event ends up spinning a nested event loop?

Comment 1

[Karl Tomlinson (:karlt)]

Do we need to know every time the plugin enters a nested event loop, even if it exits quickly?

I wonder whether we can schedule an event on a timeout before calling  g_main_context_iteration.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That sounds good.  I'll take a look at what chromium does sometime soon.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Mochitest of clipboard interaction

With OOPP pref'd on, this test hangs firefox (until the hang detector kicks in).  With it off, it passes.

Requesting karlt r? to check use of Gtk2 clipboard API in testplugin.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 425925 [details] [diff] [review]
Mochitest of clipboard interaction

>+/**
>+ * Get the current clipboard item as text.  If the clipboard item
>+ * isn't text, the returned value is undefined.
>+ */
>+std::string pluginGetClipboardText(InstanceData* instanceData);

Nit:  This seems like a static method really.
I can't think of any situation where instanceData would be used, so it probably doesn't need to be there.  NVM if you think we should leave it there.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1: Detect nested glib event loops in the plugin subprocess.

This is just the nesting detector; I'll need wait for bug 538918 to land before doing the browser-event-processing side of things.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 2: Periodically unblock the parent to allow it to process events while the plugin subprocess is in a nested event loop

Actually, this doesn't overlap with jimm's patch as much as I thought it might.

This patch fixes the mochitest, but ... it takes a *long* time to get the clipboard text in the plugin process, about 4 seconds.  I'm not sure what's going on, will look into it tomorrow.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 2: Periodically unblock the parent to allow it to process events while the plugin subprocess is in a nested event loop, v2

This is substantially the same as v1, but fixes the strange 4-second copy/paste lag.  There's an |// XXX| comment in PluginModuleParent.cpp that explains the problem.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 425937 [details] [diff] [review]
Part 1: Detect nested glib event loops in the plugin subprocess.

For battery life reasons, etc., it would be good to only schedule the 90ms
repeating timer when the browser is blocked on the plugin.  It also only seems
necessary to ProcessBrowserEvents when the browser is blocked (on an rpc
call).  (If the browser is not blocked, then it is already processing events.)
IIRC the glib functions here a thread-safe (or at least can be used in a
thread safe way) so I think it should be possible to schedule the events from
the ipc message thread.

Is there any chance that a event might get processed after the
PluginModuleChild is destroyed?  If so the timer needs to be cancelled (or
hold a reference to the PluginModuleChild).

>+                       static_cast<gpointer>(this),

This is a cast to a base class (if you consider void* a base class), so the
explicit cast is not necessary.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
> Is there any chance that a event might get processed after the
> PluginModuleChild is destroyed?  If so the timer needs to be cancelled (or
> hold a reference to the PluginModuleChild).
> 

No.  In the child process, the "IO thread" has main(), the plugin thread and PluginModuleChild are created on main(), and main() joins the plugin thread before deleting it and the PluginModuleChild.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 426054 [details] [diff] [review]
 Part 2: Periodically unblock the parent to allow it to process events while the plugin subprocess is in a nested event loop, v2

>+    // problem is, the native appshell is just an observer of our
>+    // nsThread, and processes native events as a side effect of
>+    // nsThread::ProcessNextEvent().  Since native events are the ones

That is disappointing and unhelpful.

>+        // this shouldn't return false, and there's not really
>+        // anything we can/should do if it does
>+        NS_ProcessNextEvent(nsnull, PR_FALSE);

This is going to return false often enough now.
Either remove or update the comment.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1: Detect nested glib event loops in the plugin subprocess, v2

This uses the newly added IPDL EnteredCxxStack()/ExitedCxxStack() callbacks to schedule/deschedule the timers.  EnteredCxxStack() is called when the first C++ stack frame related to IPC is pushed on the stack; this includes both incoming and outgoing messages.  We schedule the detector timer on EnteredCxxStack().  All incoming messages result in the browser blocking on the plugin, because the browser only sends RPC messages.  Outgoing messages can also result in the browser blocking on the plugin if the browser re-enters the out-call.

ExitedCxxStack() is invoked when the last C++ stack frame related to IPC is popped off the stack.  Whichever timer is active is canceled then, either the detector, or the "process browser events" timer if a nested loop was spun.

In the (hopefully) common case of no nested event loops, the glib timer callbacks will never be invoked.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 426632 [details] [diff] [review]
 Part 1: Detect nested glib event loops in the plugin subprocess, v2

Looks good.

>+#if defined(MOZ_WIDGET_GTK2)
>+    guint mTimerId;
>+#endif

Can this be given an more descriptive name such as mNestingTimerId or similar?

Can you also add a comment please to explain that this will either be the
timer for DetectNestedEventLoop (x)or ProcessBrowserEvents?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/projects/electrolysis/rev/64878c12c181
http://hg.mozilla.org/projects/electrolysis/rev/620a72b9de98
http://hg.mozilla.org/projects/electrolysis/rev/e136d6b6f5ce

Comment 14

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/569dede83071
http://hg.mozilla.org/mozilla-central/rev/d553de3fad40
http://hg.mozilla.org/mozilla-central/rev/a31c15677467
http://hg.mozilla.org/mozilla-central/rev/122641d81c49