550118 - Try to avoid processing XPCOM events during PluginModuleParent::AnswerProcessSomeEvents()

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

See bug 550026 for an example of this leading to trouble.

It's difficult to impossible to prevent this from happening in general, but there are two approaches to mitigating the problem
 - call g_main_context_iteration() directly
 - call nsBaseAppShell::DoProcessNextNativeEvent() directly

The former is more likely to lead to XPCOM events being processed and leads to gtk2/qt code divergence, but the latter is a bigger sw engineering challenge.  See bug 550026 for a fuller discussion.

bsmedberg, thoughts?

Comment 1

[Karl Tomlinson (:karlt)]

As indicated above, this seems to be something that is going to be very very difficult to win on.

Native events may cause arbitrary javascript to run.
I assume a javascript alert will start a nested event loop.

There may be performance reasons to try to prefer native events, but our event processing code should probably do that anyway (not just in this particular situation).

Comment 2

[Benjamin Smedberg]

Well, there are two issues:

1) we want to match the current behavior that plugins would have when they spin a nested event loop. Normally if they show a modal dialog or something they would spin the GTK event loop with g_main_context_iteration or something similar, correct?

2) If their code re-enters the host (e.g. through npruntime) and causes mozilla to display a modal alert/dialog, we may spin a nested XPCOM event loop. I thought we had some kind of mechanism where only certain types of events are allowed through in that case, but I don't see it.

Comment 3

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

(In reply to comment #2)
> 1) we want to match the current behavior that plugins would have when they spin
> a nested event loop. Normally if they show a modal dialog or something they
> would spin the GTK event loop with g_main_context_iteration or something
> similar, correct?
> 

That's a very compelling argument for a v0 fix that spins the glib event loop.  Patch coming up.

Comment 4

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

Attachment: Try to not process XPCOM events when the plugin process spins a nested glib event loop

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 430402 [details] [diff] [review]
Try to not process XPCOM events when the plugin process spins a nested glib event loop

>     for (int i = 0; i < kMaxChancesToProcessEvents; ++i)
>-        NS_ProcessNextEvent(nsnull, PR_FALSE);
>+        g_main_context_iteration(NULL, false);

I don't actually expect this to make a big difference to which events get run, but it makes sense that a glib event loop is spun in response to a glib event loop detected in the plugin.

Can you use the return value of g_main_context_iteration to exit the loop early when there are no events processed, please?
(One day we may even want to return that to the plugin process as an indication of how soon to let the parent process more events.)

I would s/false/FALSE/ for the gboolean to keep types consistent.

Comment 6

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

(In reply to comment #5)
> (From update of attachment 430402 [details] [diff] [review])
> >     for (int i = 0; i < kMaxChancesToProcessEvents; ++i)
> >-        NS_ProcessNextEvent(nsnull, PR_FALSE);
> >+        g_main_context_iteration(NULL, false);
> 
> Can you use the return value of g_main_context_iteration to exit the loop early
> when there are no events processed, please?
> (One day we may even want to return that to the plugin process as an indication
> of how soon to let the parent process more events.)
> 

Oh right, makes total sense now that we're in the glib world.

> I would s/false/FALSE/ for the gboolean to keep types consistent.

Oops, typo.

Comment 7

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

After fixing this up, I went back and played with testCopyPaste.html to see if 20 iterations was still a good number.  It seems to be; the first time I load the test page, it takes between 29-32 iterations (2 ProcessSomeEvents() calls) before the plugin drops out of the nested loop.  Subsequent loads after the first need <= 20 (1 ProcessSomeEvents() call).

Dunno if synchronous copy/paste is a good test case, but it's all we got :S.

Comment 8

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

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