1594989 - Fix nsSubDocumentFrame::GetDocShell for Fission

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This is used for two things: intrinsic sizing of an <object> or <embed> pointing to an SVG image and something about mouseout handling. The former presumably needs to work even if the SVG is not same-origin, while the latter might maybe work ok with fission; I don't know how to exercise that codepath.

Comment 2

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 3

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

SVG embeds may be sized incorrectly until this is fixed.

Comment 4

[Sean Voisen (:svoisen)]

Cameron: Would be able to take a look at this one?

Comment 5

[Cameron McCormack (:heycam)]

Taking.

Comment 6

[Cameron McCormack (:heycam)]

Hi Edgar,

I'm looking at this bug, and one of the two uses of nsSubDocumentFrame::GetDocShell is in EventStateManager::NotifyMouseOut here:

https://searchfox.org/mozilla-central/rev/1b95a0179507a4dc7d4b0c94c2df420dc1a72885/dom/events/EventStateManager.cpp#4321

We get in here when a WidgetMouseEvent is dispatched to some element when mLastOverElement is an iframe, and we need to cause the iframe's document to do any processing for a mouse leaving event (dispatch mouseexit, mouseout, update content state).

It seems like I should do something like this:

if (auto remote = BrowserParent::GetFrom(wrapper->mLastOverElement)) {
  remote->CallNotifyMouseOut();
}

but in the child process when I call NotifyMouseOut I would need a WidgetMouseEvent so that it can look up the right OverOutElementsWrapper. Do you know what I should be doing here? Is there some way to map the WidgetMouseEvent in the parent to the right one to use in the child? Or is there some other way I can look up the OverOutElementsWrapper to use in the child in this case?

Comment 7

[Edgar Chen [:edgar]]

(In reply to Cameron McCormack (:heycam) from comment #6)

one of the two uses of nsSubDocumentFrame::GetDocShell is in EventStateManager::NotifyMouseOut here:

https://searchfox.org/mozilla-central/rev/1b95a0179507a4dc7d4b0c94c2df420dc1a72885/dom/events/EventStateManager.cpp#4321

nsSubDocumentFrame::GetDocShell in EventStateManager::NotifyMouseOut is for the case that iframe lives in the same process as its parent frame, we would go this code path to notify the iframe that mouse out.

We get in here when a WidgetMouseEvent is dispatched to some element when mLastOverElement is an iframe, and we need to cause the iframe's document to do any processing for a mouse leaving event (dispatch mouseexit, mouseout, update content state).

It seems like I should do something like this:

if (auto remote = BrowserParent::GetFrom(wrapper->mLastOverElement)) {
  remote->CallNotifyMouseOut();
}

For the case the frame lives in a different process as its parent frame, the cross process mouseout event dispatching would be handled in EventStateManager::DispatchMouseOrPointerEvent and EventStateManager::DispatchCrossProcessEvent:

https://searchfox.org/mozilla-central/rev/d6d8fcc22c3820f2ae08229e0d37be19fba74db9/dom/events/EventStateManager.cpp#4222-4242
https://searchfox.org/mozilla-central/rev/d6d8fcc22c3820f2ae08229e0d37be19fba74db9/dom/events/EventStateManager.cpp#1315-1341

So I think we don't need to do anything more for that.

(Note: there is a known issue for nested oop iframe, see bug 1646370)

Comment 8

[Cameron McCormack (:heycam)]

Attachment: test

Thanks. I wrote this test to demonstrate how mouseout is not dispatched properly due to a cross-process iframe. Will that be fixed as part of bug 1646370 or do I still need to do something here?

Comment 9

[Cameron McCormack (:heycam)]

For the second caller (intrinsic sizing of object and embed), this is dependent on bug 1614524, so I'll wait until that's done before fixing that.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Cameron McCormack (:heycam) from comment #8)

Thanks. I wrote this test to demonstrate how mouseout is not dispatched properly due to a cross-process iframe.

The cross-process event dispatching is controlled in the parent process, content process doesn't know how to dispatch a cross-process event.
synthesizeMouseAtCenter generates the event directly in the caller's process, i.e. the process of top-level window in this case, so it won't generate the corresponding event in iframe's process. If we would like to test cross-process event dispatching, we could call synthesizeNativeMouse* which will generate the event in parent process.

There are some similar tests, like https://searchfox.org/mozilla-central/source/dom/events/test/test_mouse_enterleave_iframe.html and https://searchfox.org/mozilla-central/source/dom/events/test/browser_mouse_enterleave_switch_tab.js.

However, I modified your test by using synthesizeNativeMouseMove, it still doesn't work.
It looks like the mouseout/mouseleave doesn't work properly on the root element, filed bug 1653949.

do I still need to do something here?

No. I will take look at it in bug 1653949. Thanks for finding this bug.

Comment 11

[Cameron McCormack (:heycam)]

Bug 1658343 is for fixing the intrinsic sizing of object and embed.

Comment 12

[Neha Kochar [:neha]]

Cameron, since the first caller is fixed in bug 1653949 and the second will be fixed in Bug 1658343, should this bug be closed or is there something still left to do here?

Comment 13

[Cameron McCormack (:heycam)]

Nothing left to do here, I think. nsSubDocumentFrame::GetDocShell remains, as it's still called by EventStateManager::NotifyMouseOut, but presumably after bug 1653949 it's fine for it to return null for an OOP frame.