All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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?
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

9 years ago
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.
(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.
Created attachment 430402 [details] [diff] [review]
Try to not process XPCOM events when the plugin process spins a nested glib event loop
Assignee: nobody → jones.chris.g
Attachment #430402 - Flags: review?(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.
Attachment #430402 - Flags: review?(karlt) → review+
(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.
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.
http://hg.mozilla.org/mozilla-central/rev/dc7f0b162e9c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.