Fix failing layout/base/tests/test_bug603550.html for Fission
Categories
(Core :: DOM: Copy & Paste and Drag & Drop, defect, P3)
Tracking
()
Fission Milestone | Future |
People
(Reporter: neha, Assigned: masayuki)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Reporter | ||
Comment 1•4 years ago
|
||
Masayuki, you might be familiar with this test? :)
Assignee | ||
Comment 2•4 years ago
|
||
Investigating, but needs more time to check my idea...
Assignee | ||
Comment 3•4 years ago
|
||
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 | ||
Comment 4•4 years ago
|
||
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.
Comment 5•4 years ago
|
||
(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 thatBrowserParent
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
- https://searchfox.org/mozilla-central/source/dom/events/test/pointerevents/test_pointercapture_xorigin_iframe.html
- https://searchfox.org/mozilla-central/source/dom/tests/mochitest/pointerlock/test_pointerlock_xorigin_iframe.html
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.
Comment 6•4 years ago
|
||
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.
Comment 7•4 years ago
|
||
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.
Comment 8•4 years ago
|
||
(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.
Comment 9•4 years ago
|
||
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?
Assignee | ||
Comment 10•4 years ago
|
||
(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?
Assignee | ||
Comment 11•4 years ago
|
||
Ah, wait, I did wrong merge. I'll test it again. Sorry for the spam.
Assignee | ||
Comment 12•4 years ago
|
||
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.
Assignee | ||
Comment 13•4 years ago
|
||
(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 thatBrowserParent
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
- https://searchfox.org/mozilla-central/source/dom/events/test/pointerevents/test_pointercapture_xorigin_iframe.html
- https://searchfox.org/mozilla-central/source/dom/tests/mochitest/pointerlock/test_pointerlock_xorigin_iframe.html
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.
Assignee | ||
Comment 14•4 years ago
|
||
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.
Assignee | ||
Comment 15•4 years ago
|
||
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.
Comment 16•4 years ago
|
||
The severity field is not set for this bug.
:jstutte, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 17•4 years ago
|
||
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,
- 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) - 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)
Assignee | ||
Comment 18•4 years ago
|
||
(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.
Assignee | ||
Comment 19•4 years ago
|
||
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?
Assignee | ||
Comment 20•4 years ago
|
||
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...
Comment 21•4 years ago
|
||
(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-100You tries to synthesize a drag session with
nsIDragService
, but you also prevent default of the precedingmousedown
event. Whenmousedown
is canceled, drag session won't be started in actual operation. So,
- 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)- 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,
- mouse is captured even if prevent default on mousedown.
- mouse capture is ended after dnd session.
But I might be wrong.
Comment 22•4 years ago
|
||
Answering to comment 17.
I think that code was trying to emulate dnd and ensure the native dnd handling didn't kick in.
Assignee | ||
Comment 24•4 years ago
|
||
(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).
Comment 25•4 years ago
|
||
(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)
:
[test_bug603550.html]
fail-if = (xorigin && fission)
Description
•