1304761 - Javascript contextmenu event not fired when e10s enabled

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[ozzob1]

Attachment: e10s_contextmenu.htm

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160916101415

Steps to reproduce:

1) On DIV node, attach Javascript callback on mouseup and contextmenu event.
2) In mouseup callback, send synchronous XMLHttpRequest
3) Right click on DIV node


Actual results:

In Firefox with e10s enabled, contextmenu event is never fired (regardless of XMLHttpRequest request treatment time).
In Firefox with e10s disabled, contextmenu event is always fired.


Expected results:

Firefox with e10s enabled should have the same behavior than with e10s disabled

Comment 1

[Jim Mathies [:jimm]]

Mike, can you take a look here?

Comment 2

[Jim Mathies [:jimm]]

(Curious how serious this is.)

Comment 3

[Mike Taylor [:miketaylr]]

So, breaking the contextmenu event is a big deal, IMO.

However, I can't reproduce in this TC in DevEdition 50 on Mac.

Is the sync XHR part crucial to the test case?

Comment 4

[Loic]

I'm able to reproduce the issue with FF49/52 on Win 7.
I tested FF40/45 with e10s enabled too, same issue, so I don't think it's a regression.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Seems that this issue is only reproducible on Windows (49.0.1) with e10s enabled, I cannot reproduce it on Mac nor Linux Ubuntu. If I remove the sync XHR part, the issue is gone: the contextmenu event is fired, and the context menu is shown.

Comment 6

[Jessica Jong [:jessica] (inactive)]

After digging in more, it seems that we suppress input events during synchronous XHR (see bug 333198), but I am not sure why only some events are added to the delayed event queue when events are suppressed. Olli, could you shed some light here? Thanks.

Note that this happens only on Windows, because Windows shows context menu on mouseup, and other platforms show it on mousedown. I can reproduce this on osx + nightly (but not on official 49, weird) if I change the testcase calling syncSend() on mousedown.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Only the very minimal set of input events are being suppressed. Perhaps we're missing something there.
But why did e10s break this?

Comment 8

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Only the very minimal set of input events are being suppressed. Perhaps
> we're missing something there.
> But why did e10s break this?

It seems that it only happens in e10s due to timing of event processing. From my manual tests, in non-e10s, on mouseup, XML will suppress [1] and unsuppress[2] events, before eContextMenu is processed in nsPressShell [3]. But in e10s, eContentMenu is dispatched to content process and handled in nsPressShell before XML unsupresses events. 

By adding eContextMenu in the delayed event queue seems to solve this, but I am not really familiar with the code to know what events should (not) be added to the delayed event queue...

[1] http://hg.mozilla.org/mozilla-central/file/dc89484d4b45/dom/xhr/XMLHttpRequestMainThread.cpp#l2926
[2] http://hg.mozilla.org/mozilla-central/file/dc89484d4b45/dom/xhr/XMLHttpRequestMainThread.cpp#l2961
[3] http://hg.mozilla.org/mozilla-central/file/dc89484d4b45/layout/base/nsPresShell.cpp#l7731

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Adding eContextMenu to delayed events sounds ok to me.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 11

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa68fef276dc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbea63ccda87
add contextmenu event to the delayed event queue if it's suppressed. r=smaug

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbea63ccda87