1426154 - Use targetted message manager to communciate with content browsers

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

Marionette currently uses a message manager broadcaster to communicate
with content browsers.  This is hugely wasteful and fragile,
especially because we use the content browser’s outerWindowID as
the marker for which frame script picks up the message because it
changes when the browser suffers a remoteness change.

We should instead use a targetted
message manager.  The window refactoring work in
https://bugzilla.mozilla.org/show_bug.cgi?id=marionette-window-tracking
makes this possible.

One problem the window refactoring does not take care of is the
Marionette:listenersAttached and this.curBrowser.pendingCommands queue.
I believe we still need some version of this, in case the content
browser disappears whilst processing a message we have sent it.
We are still liable to see this despite improving the tracking of
browsers and windows.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Btw the framescript could listen for the `unload` event on the tabchildglobal to send the parent a message about its unload. That should help us to better handle such situations.

Comment 2

[Andreas Tolfsen ❲:ato❳]

We could perhaps use the unload event to tell chrome to delay, or
more likely re-send, the last message.  Provided it fires consistently,
of course.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tested that locally for a patch on a different bug, and it was working fine. Only sending the data to the parent process wasn't working that well at that time.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMessageListenerManager#addMessageListener()

Comment 4

[Andreas Tolfsen ❲:ato❳]

It looks like we might be able to employ a targetted message
manager for communicating directly with content frames without the
window tracking refactoring.  It might make sense to reverse the
relationship and try to land this first.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1426154 - Use targetted IPC messages to talk to content frames.

Prior to this patch Marionette used a broadcasting technique that
sent every IPC message to every registered content frame's message
manager.  This was implemented in GeckoDriver#sendAsync and worked
by appending the frame's outerWindowID to the message name to ensure
it reached the correct recipient.

Because it is hugely wasteful for every content frame to receive every IPC
message, this patch changes it so that we use a frame's dedicated message
manager to only channel messages it cares about.  A content frame's
direct message manager is stored in browser.Context#messageManager,
whereas GeckoDriver#mm is always the global message manager.

Review commit: https://reviewboard.mozilla.org/r/211446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211446/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1426154 - Sort message listeners lexicographically.

It is easier to review what message listeners are added and removed
when the list is sorted.

Review commit: https://reviewboard.mozilla.org/r/211448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211448/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1426154 - Refactor Marionette content frame script registration.

The Marionette:newSession IPC message is never used, and the
newSession function is consequently never called.  Since we no
longer have a need for resetting the content frame script, like
had with B2G, removing it is probably fine.

Review commit: https://reviewboard.mozilla.org/r/211450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211450/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

Marionette:deleteSession has always been misleadingly named as it
does not actually delete a Marionette session, but cleans up some
state and removes message listeners.

The forthcoming window tracking refactoring will formalise the
concepts "init" and "register" (with corresponding "uninit" and
"deregister"), so this can be seen as preparing for that change.

Review commit: https://reviewboard.mozilla.org/r/211452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211452/

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Great to see the progress on this bug Andreas! I hope that this will already fix the multi-process issues which I have seen on bug 1422769.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941180 [details]
Bug 1426154 - Use targetted IPC messages to talk to content frames.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211446/diff/1-2/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941181 [details]
Bug 1426154 - Sort message listeners lexicographically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211448/diff/1-2/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211450/diff/1-2/

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941183 [details]
Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211452/diff/1-2/

Comment 14

[Andreas Tolfsen ❲:ato❳]

Taskcluster had some kind of major cockup yesterday that ruined
the test run.  Triggered a new one.

Comment 15

[David Burns :automatedtester]

Comment on attachment 8941180 [details]
Bug 1426154 - Use targetted IPC messages to talk to content frames.

https://reviewboard.mozilla.org/r/211446/#review217508

Comment 16

[David Burns :automatedtester]

Comment on attachment 8941181 [details]
Bug 1426154 - Sort message listeners lexicographically.

https://reviewboard.mozilla.org/r/211448/#review217512

Comment 17

[David Burns :automatedtester]

Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

https://reviewboard.mozilla.org/r/211450/#review217514

Comment 18

[David Burns :automatedtester]

Comment on attachment 8941183 [details]
Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

https://reviewboard.mozilla.org/r/211452/#review217518

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941180 [details]
Bug 1426154 - Use targetted IPC messages to talk to content frames.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211446/diff/2-3/

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941181 [details]
Bug 1426154 - Sort message listeners lexicographically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211448/diff/2-3/

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211450/diff/2-3/

Comment 22

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941183 [details]
Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211452/diff/2-3/

Comment 23

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211450/diff/3-4/

Comment 24

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941183 [details]
Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211452/diff/3-4/

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211450/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/073f421c10dd
Use targetted IPC messages to talk to content frames. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/d1805bdd7b73
Sort message listeners lexicographically. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/e08b9e460de6
Refactor Marionette content frame script registration. r=automatedtester

Comment 27

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/073f421c10dd
https://hg.mozilla.org/mozilla-central/rev/d1805bdd7b73
https://hg.mozilla.org/mozilla-central/rev/e08b9e460de6