Open Bug 1677543 Opened 4 years ago Updated 4 years ago

Fix failing layout/base/tests/test_bug603550.html for Fission

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P3)

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Type:
defect

Tracking

()

Status:
ASSIGNED
Project Flags:
Fission Milestone Future

People

(Reporter: neha, Assigned: masayuki)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

No description provided.

Masayuki, you might be familiar with this test? :)

Fission Milestone: --- → M6c
Flags: needinfo?(masayuki)

Investigating, but needs more time to check my idea...

Hmm, I'm still not sure what's wrong. However, I believe that the test does not make sense at least since shipping e10s. I'll rewrite the test for synthesizing all events from the chrome process. Then, the test can check whether it works as expected in real use.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

I rewrite the test completely async, i.e., all mouse events are synthesized in the parent process and check delivered events in the content process.
https://treeherder.mozilla.org/jobs?repo=try&resultStatus=pending%2Crunning%2Cusercancel%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=0e0ef42289732c10464a410fb432d64f0640bf2e

However, in xorigin mode, even mousedown event is not delivered properly. Edgar, do you know something about it? It seems that BrowserParent does not convert the points in OOP <iframe> to parent process' points properly.

Flags: needinfo?(echen)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #4)

However, in xorigin mode, even mousedown event is not delivered properly. Edgar, do you know something about it? It seems that BrowserParent does not convert the points in OOP <iframe> to parent process' points properly.

From the log, I guess mousedown is delivered, but with a wrong information? Could you share what gets wrong in mousedown event? Thanks!

I have written a couple of tests that dispatches mouse event async, like

I didn't notice the issue, but in my case, I synthesize the event from the top-level window, and yours synthesize from the test iframe, that could possibly cause a difference.

Flags: needinfo?(echen) → needinfo?(masayuki)

In Fission, we reply on https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/dom/events/EventStateManager.cpp#1345-1349,1378-1385 to determine which process we should dispatch the mouse event to. Another possible issue is that we don't handle mouse capture well for OOP iframe.

Hmm, I think we don't handle mouse capture well, see https://codepen.io/edgarchen-the-decoder/pen/abmvQQy as an example, STR: press mouse button on red area and move mouse to upper pannel, e.g. HTML pannel.

(In reply to Edgar Chen [:edgar] from comment #7)

Hmm, I think we don't handle mouse capture well, see https://codepen.io/edgarchen-the-decoder/pen/abmvQQy as an example, STR: press mouse button on red area and move mouse to upper pannel, e.g. HTML pannel.

Filed bug 1680405, I will take a look tomorrow.

Masayuki, https://phabricator.services.mozilla.com/D98592 should make the mouse capture working on OOP iframe, could you try to apply the patch to see if this fix the issue you met in xorigin mode?

(In reply to Edgar Chen [:edgar] from comment #9)

Masayuki, https://phabricator.services.mozilla.com/D98592 should make the mouse capture working on OOP iframe, could you try to apply the patch to see if this fix the issue you met in xorigin mode?

Thank you! And really excellent!! Indeed, your patch makes my new test green! I'll request review to smaug before your patch with marking it as expecting failure in xorigin mode. Then, could you remove it in your patch?

Flags: needinfo?(masayuki)

Ah, wait, I did wrong merge. I'll test it again. Sorry for the spam.

Unfortunately, your patch does not fix the orange in the xorigin mode. I'll check it with debugger what's going on in the parent process today.

(In reply to Edgar Chen [:edgar] from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #4)

However, in xorigin mode, even mousedown event is not delivered properly. Edgar, do you know something about it? It seems that BrowserParent does not convert the points in OOP <iframe> to parent process' points properly.

From the log, I guess mousedown is delivered, but with a wrong information? Could you share what gets wrong in mousedown event? Thanks!

Oh, that's odd. In my environment (Win10), I see mousedown event is not delivered to the content process.

I have written a couple of tests that dispatches mouse event async, like

I didn't notice the issue, but in my case, I synthesize the event from the top-level window, and yours synthesize from the test iframe, that could possibly cause a difference.

Thank you for your quick work. Yeah, it makes sense. If there is no test which synthesize mouse events from OOP <iframe> via the parent process, this path may have not implemented yet.

(In reply to Edgar Chen [:edgar] from comment #6)

In Fission, we reply on https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/dom/events/EventStateManager.cpp#1345-1349,1378-1385 to determine which process we should dispatch the mouse event to. Another possible issue is that we don't handle mouse capture well for OOP iframe.

Thank you for the information. This might help me to keep investigating this.

(In reply to Edgar Chen [:edgar] from comment #8)

(In reply to Edgar Chen [:edgar] from comment #7)

Hmm, I think we don't handle mouse capture well, see https://codepen.io/edgarchen-the-decoder/pen/abmvQQy as an example, STR: press mouse button on red area and move mouse to upper pannel, e.g. HTML pannel.

Filed bug 1680405, I will take a look tomorrow.

Thank you again! Unfortunately it does not fix my issue directly, but I guess that it's necessary. So, feel free to land it before my patch.

Hmm, when it fails to receive the first mousedown event, it seems that the testing <iframe> is loaded and reflowed before its parent document in another remote process. It seems that there is no way to wait parent document's reflow...

But I still don't find why the first mousemove event isn't delivered correctly.

Ah, the latter is fixed by the Edgar's patch!

OK, I'll look for a way to fix the former case, and I still see the failure even with the async test. I'll keep investigating.

Depends on: 1680405

The severity field is not set for this bug.
:jstutte, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jstutte)

I'm now really confused what smaug tried to check with the last part of the test:
https://searchfox.org/mozilla-central/rev/8d04c3f5332d470eeae5aa3dc0ed132359a339c1/layout/base/tests/test_bug603550.html#55-71,87-91,97-100

You tries to synthesize a drag session with nsIDragService, but you also prevent default of the preceding mousedown event. When mousedown is canceled, drag session won't be started in actual operation. So,

  1. Did you try to check whether mouse capture is ended by ending drag session? (i.e., the mousedown event listener is accidentally alive at the last test)
  2. Did you try to check whether web app's DnD emulation won't cause mouse capture? (i.e., synthesizing a drag session with nsIDragSession is not necessary)
Flags: needinfo?(bugs)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #16)

The severity field is not set for this bug.

I'm still not sure the failure detects a bug of Fission or a bug of test framework. Therefore, I've not set the severity, but the priority of investigation is high.

Okay, I got what occurs. Even if mousedown is canceled in a content process, the XUL element in the parent process keeps capturing the mouse for the process. Therefore, even if mousemove event is fired outside the outer-most document of a remote process, mousemove events are fired on the outer-most document's Document node. In the xorigin mode, the testing document becomes the outer-most document different from the other modes. Probably, mousemove events outside the document should be discarded in remote process if it is not capturing mouse events. What do you think, smaug?

Severity: -- → S3
Flags: needinfo?(jstutte)
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All

Perhaps, it'd be nicer to release capturing mouse events in the parent process when it's released in a content process. However, it's difficult, e.g., even if mousedown is canceled in a content process, following some mousemove events which are fired outside the document may be fired in the content process until mousedown cancellation is handled by the parent process. So, outer content may fail to receive only some mousemove events and that may cause odd behavior...

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #17)

I'm now really confused what smaug tried to check with the last part of the test:
https://searchfox.org/mozilla-central/rev/8d04c3f5332d470eeae5aa3dc0ed132359a339c1/layout/base/tests/test_bug603550.html#55-71,87-91,97-100

You tries to synthesize a drag session with nsIDragService, but you also prevent default of the preceding mousedown event. When mousedown is canceled, drag session won't be started in actual operation. So,

  1. Did you try to check whether mouse capture is ended by ending drag session? (i.e., the mousedown event listener is accidentally alive at the last test)
  2. Did you try to check whether web app's DnD emulation won't cause mouse capture? (i.e., synthesizing a drag session with nsIDragSession is not necessary)

It looks like it tries to test two things,

  1. mouse is captured even if prevent default on mousedown.
  2. mouse capture is ended after dnd session.

But I might be wrong.

Answering to comment 17.
I think that code was trying to emulate dnd and ensure the native dnd handling didn't kick in.

Flags: needinfo?(bugs)

Any updates, Masayuki?

Flags: needinfo?(masayuki)

(In reply to Neha Kochar [:neha] from comment #23)

Any updates, Masayuki?

I'll be back to bug 1681647 blocking this bug. But I was blocked by odd result of xorigin testcase.

I'm thinking that there could be some real issues at xorigin document boundaries, but I feel that this failure is caused by problem of both test framework and the test itself. So, I don't think that this should block to ship Fission (anyway, even after fixing this bug, there is a web-compat issue that is where should mousemove event be fired on. Gecko fires on the document node, but Blink does not, IIRC, and this issue is not related to Fission).

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #24)

I'm thinking that there could be some real issues at xorigin document boundaries, but I feel that this failure is caused by problem of both test framework and the test itself. So, I don't think that this should block to ship Fission (anyway, even after fixing this bug, there is a web-compat issue that is where should mousemove event be fired on. Gecko fires on the document node, but Blink does not, IIRC, and this issue is not related to Fission).

Thanks for the update. In that case, I will move this bug to our Fission Future milestone so it doesn't block shipping Fission MVP.

This test is currently annotated as failing for (xorigin && fission):

https://searchfox.org/mozilla-central/rev/d5e98892f9553f9113e69c585308008e681e19c9/layout/base/tests/mochitest.ini#65-66

[test_bug603550.html]
fail-if = (xorigin && fission)
Fission Milestone: M6c → Future
You need to log in before you can comment on or make changes to this bug.