[e10s] Refine event dispatching state between processes

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

Currently, event dispatching state between processes are manged with some bool flags.  However, it's difficult to understand especially if we need to add new flags.  So, I'd like to clean up it before fixing bug 1257617.
Comment on attachment 8882977 [details]
Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState

https://reviewboard.mozilla.org/r/153942/#review159096
Attachment #8882977 - Flags: review?(bugs) → review+
Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159098

::: dom/events/Event.cpp:460
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
>  Event::StopCrossProcessForwarding()
>  {
> -  mEvent->StopCrossProcessForwarding();
> +  mEvent->PreventDispatchedToRemoteProcess();

I don't like PreventDispatchedToRemoteProcess().
StopCrossProcessForwarding is easier to understand, IMO.

::: dom/ipc/TabChild.cpp:1922
(Diff revision 1)
>    }
>  
>    // If a response is desired from the content process, resend the key event.
>    // If mAccessKeyForwardedToChild is set, then don't resend the key event yet
>    // as RecvHandleAccessKey will do this.
> -  if (localEvent.mFlags.mWantReplyFromContentProcess) {
> +  if (localEvent.NeedsToReplyToParentProcess()) {

Also this. I this the existing name is easier to understand.
Attachment #8882978 - Flags: review?(bugs) → review-
Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159100

Calling MarkAsPostedToRemoteProcess(); everywhere separately is error prone.
Couldn't we use ParamTrait<WidgetEvent>::Write for this. It would always set the flag

::: widget/BasicEvents.h:158
(Diff revision 1)
>    // in the parent process after the content process has handled it. Useful
>    // for when the parent process need the know first how the event was used
>    // by content before handling it itself.
>    bool mWantReplyFromContentProcess : 1;
> +  // If mPostedToRemoteProcess is true, the event has been posted to the
> +  // remote process (but it's not handled yet if it's not a duplicated evnet

event
Attachment #8882979 - Flags: review?(bugs) → review-
Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159366

::: dom/xbl/nsXBLWindowKeyHandler.cpp:551
(Diff revision 2)
>  
>    if (!HasHandlerForEvent(aEvent)) {
>      return;
>    }
>  
> -  // Inform the child process that this is a event that we want a reply
> +  // If this event hadn't been marked as IsCrossProcessForwardingStopped,

s/hadn't been/wasn't/

::: widget/BasicEvents.h:246
(Diff revision 2)
> +  {
> +    mNoRemoteProcessDispatch = false;
> +    mWantReplyFromContentProcess = true;
> +    // Stop propagation in this process.  Other event handlers should handle
> +    // the event with reply event.
> +    StopImmediatePropagation();

This is super confusing. Why does MarkAsWaitingReplyFromRemoteProcess have this kind of side effect to call StopImmediatePropagation()?

So, please remove StopImmediatePropagation() and let the caller of MarkAsWaitingReplyFromRemoteProcess() to call also that when needed.
Attachment #8882978 - Flags: review?(bugs) → review+
Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159370

It is still a bit unclear to me what is the end goal with this all. Doesn't seem to make code much easier to follow. But r+ anyhow, with those nits fixed.

::: commit-message-9f130:5
(Diff revision 2)
> +Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process r?smaug
> +
> +Currently, it's not been managed yet that whether an event is posted to at least one remote process.  So, for managing the state, BaseEventFlags should have a new bool flag and WidgetEvent and BaseEventFlags should have helper methods for it.
> +
> +Additionally, this fixes a bug of nsGUIEventIPC.h. In a lot of ParamTraits, static_cast<Foo> is used for using base class's ParamTraits.  However, it causes creating temporary instance with copy constructor.  Therefore, WidgetEvent::MarkAsPostedToRemoteProcess() call in ParamTraits<mozilla::WidgetEvent>::Write() didn't work as expected and that must have made bad damage to the performance.

Well, probably not bad damage. More like very small, just one memcpy.

::: dom/ipc/TabParent.h:461
(Diff revision 2)
> +  void SendRealTouchEvent(WidgetTouchEvent& aEvent);
>  
> -  bool SendRealKeyEvent(mozilla::WidgetKeyboardEvent& aEvent);
> +  void SendPluginEvent(WidgetPluginEvent& aEvent);
>  
> -  bool SendRealTouchEvent(WidgetTouchEvent& aEvent);
> +  /**
> +   * Different from above Send*Event(), these methods return true if the

Confusing return value. 
But I don't have now good suggestions to improve this.
Perhaps using some enum for the return value would make this less weird.

::: dom/ipc/TabParent.cpp:1182
(Diff revision 2)
>    uint64_t blockId;
>    ApzAwareEventRoutingToChild(&guid, &blockId, nullptr);
>    aEvent.mRefPoint += GetChildProcessOffset();
> -  return PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  DebugOnly<bool> ret =
> +    PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  NS_ASSERTION(ret, "PBrowserParent::SendMouseWheelEvent() failed");

Very unusual style to use NS_ASSERTION _and_ MOZ_ASSERT.
Do we really need NS_ASSERTION ?
Attachment #8882979 - Flags: review?(bugs) → review+
Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159366

> This is super confusing. Why does MarkAsWaitingReplyFromRemoteProcess have this kind of side effect to call StopImmediatePropagation()?
> 
> So, please remove StopImmediatePropagation() and let the caller of MarkAsWaitingReplyFromRemoteProcess() to call also that when needed.

Hmm, I think that it's really odd to keep handling an event in the process even when a former listener wants reply from a remote process. On the other hand, indeed, it's hard to guess that it'll stop propagation from its name...
Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159418

::: dom/ipc/TabParent.cpp:1182
(Diff revision 2)
>    uint64_t blockId;
>    ApzAwareEventRoutingToChild(&guid, &blockId, nullptr);
>    aEvent.mRefPoint += GetChildProcessOffset();
> -  return PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  DebugOnly<bool> ret =
> +    PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  NS_ASSERTION(ret, "PBrowserParent::SendMouseWheelEvent() failed");

Well, it must be unexpected result for the callers if it fails to send the event to a remote process without already destroyed TabParent or something. Therefore, I used NS_ASSERTION for showing to enter unexpected path, and using MOZ_ASSERT which detects a bug of calling MarkAsPostedToRemoteProcess in ParamTraits<WidgetEvent>::Write.

Is it also odd to use NS_WARNING_ASSERTION?
oh, perhaps NS_WARNING_ASSERTION would be better than NS_ASSERTION
Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159370

> Well, probably not bad damage. More like very small, just one memcpy.

I'll remove it from the commit message.

> Confusing return value. 
> But I don't have now good suggestions to improve this.
> Perhaps using some enum for the return value would make this less weird.

I agree. I'll file a followup bug.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/308c1f2050b7
part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState r=smaug
https://hg.mozilla.org/integration/autoland/rev/13dbbebe646a
part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process r=smaug
https://hg.mozilla.org/integration/autoland/rev/44f5ac7a16e4
part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process r=smaug
https://hg.mozilla.org/mozilla-central/rev/308c1f2050b7
https://hg.mozilla.org/mozilla-central/rev/13dbbebe646a
https://hg.mozilla.org/mozilla-central/rev/44f5ac7a16e4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.